-
Notifications
You must be signed in to change notification settings - Fork 193
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
add px-based sizes #493
add px-based sizes #493
Conversation
Run & review this pull request in StackBlitz Codeflow. |
What would you think about IMO "px" is an implementation detail and the prop name should make the intention clear: to prevent the size from changing when user context changes (device width, base font size, etc). |
I like that idea. Makes a lot of sense. |
Naming, always a fun thing, amiright?! I think I preferred Or maybe we make this a breaking change and these pixel values become the default |
I figured this wasn't a concern since "--size-relative-1" is not "--size-rel-1". Most of the existing props seem to use full words (content vs cont, shadow vs shad, animation vs ani, radius vs radi, weight vs wgt, etc)
Sounds good to me. Or
I prefer that the default size type is adaptive. IMO, you should have to explicitly override user preferences by adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything in this PR looks good, but lets settle on the naming. I'll bring it up in Discord too, get more eyes on it.
We don't need a learning curve, please! "px" should be included in the name 🙂 |
I think I prefer keeping And while I did say earlier that I like the name So if we picked the intent-based route, I would go with |
From the early days of this library, we took the approach to keep things simple with a minimal learning curve. While options like |
I prefer the original 'px' abbreviation over 'static' because it's shorter and will make immediate sense what the size will be based on. |
Just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you everyone for your comments and Brian for the rad contributions 🤘🏻
available in |
resolves #484