feat: rework fontsize classes with pixels to rem#1116
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR standardises typography across multiple Vue components by replacing pixel-based utility classes (e.g. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
uno.config.ts (1)
43-46: Consider specifying alineHeightalongsidefontSize.The built-in Wind4 text size tokens (e.g.
xs,sm) typically define bothfontSizeandlineHeightto ensure consistent vertical rhythm. OmittinglineHeighthere meanstext-xxswill inherit whatever line-height is set by the parent, which may produce inconsistent results across the various contexts where it's now used.Also worth noting: this single
0.625remtoken replaces classes that previously ranged fromtext-[8px]throughtext-[11px]. Elements that were intentionally sized at 8px or 9px will now render larger (10px-equivalent), and those at 11px will render smaller. Please confirm that collapsing these into one size is the desired visual outcome.Suggested addition of lineHeight
text: { // This should add to the existing Wind4 preset text sizes - xxs: { fontSize: '0.625rem' }, + xxs: { fontSize: '0.625rem', lineHeight: '1rem' }, },
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
it was a deliberate decision to use px for font size, to allow browsers to scale font size separately from ui but we probably shouldn't hard code px in the classes |
|
@danielroe that one changed the spacing (padding/margin/gap/sizes) to be pixel based. Font size is still rem based, so this is good. |
|
@danielroe there seems to be a bit of confusion here - we currently use Then it could have been changed to
|
|
ah - my apologies! |
|
Looks great, thanks for working on it ❤️ Would you like to continue this part and as a result get rid of the smallest dimensions? Seems like minor changes to the layout or related elements may be needed in some places, but I hope it would be a very small part |
@alexdln You are welcome! |
|
In some places, the logic of the content structure may be lost. For example if both the text and the additional text will have the same size after update. In some places, an icon can take away attention and look visually cumbersome next to small text. And yes, remove as much classes as possible (honestly even 12px seems crazy to me, and here it's 8px) Would you create an issue or would you prefer that I create one? |
This PR is a proposal so please feel free to give your feedback.
I noticed that there were a bunch of text fontsize classes used as
text-[8px],text-[9px],text-[10px]andtext-[11px].First I wanted to change font sizes from pixels to rem for better scalability and a11y.
Then I choose the rem size of
0.625rem, which is10pxtaken with base is16px, since this was the most common usedtext-[*px]classes in the project.To make it easier to use and more consistent I extended the Uno-CSS theme with a new text class
text-xxs. And applied this overall in the project.