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

add px-based sizes #493

Merged
merged 4 commits into from
Apr 17, 2024
Merged

add px-based sizes #493

merged 4 commits into from
Apr 17, 2024

Conversation

Brian-Pob
Copy link
Contributor

  • adds px-based size props in props.sizes.js
  • changes the text in the Sizes section to include px-based sizes
  • Adds px-based size props next to the original props
  • updates the test to account for added props

resolves #484

Copy link

stackblitz bot commented Apr 8, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@Brian-Pob
Copy link
Contributor Author

Preview of changes

@dav-is
Copy link

dav-is commented Apr 11, 2024

What would you think about --size-static-1 to match the other types ("fluid", "relative")?

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).

@Brian-Pob
Copy link
Contributor Author

What would you think about --size-static-1 to match the other types ("fluid", "relative")?

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.

@argyleink
Copy link
Owner

Naming, always a fun thing, amiright?!

I think I preferred --size-px-1 more than --size-static-1, it's shorter and has no surprises. but if we go down this route, what do y'all think of --size-abs-1, since CSS calls these kinds of units absolute (not static)?

Or maybe we make this a breaking change and these pixel values become the default --size-1? Then we rename the current rem based sizes to something else like --size-adaptive-1?

@dav-is
Copy link

dav-is commented Apr 11, 2024

it's shorter

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)

what do y'all think of --size-abs-1, since CSS calls these kinds of units absolute (not static)?

Sounds good to me. Or --size-absolute-1

these pixel values become the default --size-1

I prefer that the default size type is adaptive. IMO, you should have to explicitly override user preferences by adding -abs like you suggest. This library has already established a preference for rem that other libraries do not. That default forces the developer to consider using rem units, where with other libraries they would have used px without a second thought. I think using px everywhere is more harmful than using rem everywhere.

Copy link
Owner

@argyleink argyleink left a 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.

@mobalti
Copy link
Contributor

mobalti commented Apr 11, 2024

We don't need a learning curve, please! "px" should be included in the name 🙂

@Brian-Pob
Copy link
Contributor Author

I think I prefer keeping rem sizes as the default. As for naming these sizes, I actually prefer having the whole name there so that I don't need to remember what the shortened form was ("was it size-abs or size-ab?").

And while I did say earlier that I like the name size-static, I actually do prefer size-px since it is shorter and easier to remember. I think it still gets the idea across that what you are getting are the Open-Props sizes in non-changing px units.

So if we picked the intent-based route, I would go with size-absolute fully spelled out. Otherwise, I think I ultimately prefer size-px. Just my thoughts!

@mobalti
Copy link
Contributor

mobalti commented Apr 12, 2024

From the early days of this library, we took the approach to keep things simple with a minimal learning curve. While options like --size-abs or --size-static may seem more meaningful, their significance depends on your background. I believe --size-px has a lower learning curve

@dj0le
Copy link

dj0le commented Apr 12, 2024

I prefer the original 'px' abbreviation over 'static' because it's shorter and will make immediate sense what the size will be based on.

@sorvell
Copy link

sorvell commented Apr 13, 2024

Just px-*. Compact and clear.

Copy link
Owner

@argyleink argyleink left a 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 🤘🏻

@argyleink argyleink merged commit 00e0242 into argyleink:main Apr 17, 2024
2 checks passed
@argyleink
Copy link
Owner

available in v1.7.3 🎉

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 Spacing Props
6 participants