Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: add Icon schema #152

Merged
merged 6 commits into from
Oct 3, 2023
Merged

Feat: add Icon schema #152

merged 6 commits into from
Oct 3, 2023

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Oct 2, 2023

This would close #149

Default values have been set as:
mimeType: 'svg'
pixelDensity: 1,
size: 'small'

Should tests for Icon be added on this ticket??

large = 2;
}

enum PixelDensity {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is horrible but you can't use numbers as elements of an enum

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha! I guess so, maybe align the field numbers for sanity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enums must start at 0 :'|

Copy link
Member

@achou11 achou11 Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it work if you do something like 1x, 2x (or maybe x1, x2)? not sure if any better but just an idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the second option works. I'm adding it since its less confusing to look at...

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, all looks fine to me. Some small suggestions, nothing blocking.

src/lib/decode-conversions.ts Outdated Show resolved Hide resolved
src/lib/decode-conversions.ts Outdated Show resolved Hide resolved
src/lib/decode-conversions.ts Show resolved Hide resolved
large = 2;
}

enum PixelDensity {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha! I guess so, maybe align the field numbers for sanity?

proto/icon/v1.proto Outdated Show resolved Hide resolved
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, all looks fine to me. Some small suggestions, nothing blocking.

tomasciccola and others added 4 commits October 3, 2023 09:46
Co-authored-by: Gregor MacLennan <[email protected]>
in protobufs, enums need to start at 0, so we can't align the numbers...
@tomasciccola tomasciccola merged commit dacb41e into main Oct 3, 2023
6 checks passed
@tomasciccola tomasciccola deleted the feat/icon branch October 3, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Icon schema
3 participants