[WB-1927]: Update wonder-blocks-typography to use rems #2532
Conversation
🦋 Changeset detectedLatest commit: 3b81908 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
Size Change: +71 B (+0.07%) Total Size: 103 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-phwmmkaank.chromatic.com/ Chromatic results:
|
marcysutton
force-pushed
the
FEI-4065-rems
branch
from
32759cb to
033d1db
Compare
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (534d36f) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2532"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2532 |
marcysutton
force-pushed
the
FEI-4065-rems
branch
4 times, most recently
from
ec5be22 to
df07a46
Compare
marcysutton
changed the title
marcysutton
force-pushed
the
FEI-4065-rems
branch
from
df07a46 to
08aa092
Compare
|
I got this PR ready to review by decoupling other work from it, namely updating REMs in webapp! The direct work that will follow this PR is integrating these |
| fontSize: sizing.size_360, | ||
| lineHeight: sizing.size_400, |
There was a problem hiding this comment.
suggestion: This is something that I never did when creating the font tokens, but I think it would be better to change these styles to use font tokens directly, then change those tokens to map to the sizing tokens instead. That way consumers that use the font tokens would benefit from this change as well.
There was a problem hiding this comment.
Cool, makes sense! I'll take a look.
There was a problem hiding this comment.
Related question on one-off tokens for these: https://github.com/Khan/wonder-blocks/pull/2532/files#r2047554570
| xxLarge: sizing.size_280, | ||
| xLarge: sizing.size_240, | ||
| large: sizing.size_200, | ||
| medium: sizing.size_160, |
There was a problem hiding this comment.
This one will need to change in a TB theme file: Foundations uses sizing.size_180 for medium: https://www.figma.com/design/9seilz0fBgo3QUj6eXM0yR/%E2%9A%A1%EF%B8%8F-Foundations?node-id=52-3&t=NFXovVlCFbYhQHvm-0
I'll check for others when I get to that part (in a future PR).
There was a problem hiding this comment.
Agreed! you might want to look at the CSS vars refactor that I've been doing + the TB theming PR, as probably most of this stuff will get moved to the theme/ folder to convert these tokens as CSS variables + create the tb version of these tokens. We can chat later about this!
.changeset/clever-timers-whisper.md
Outdated
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| "@khanacademy/wonder-blocks-typography": minor | |||
| "@khanacademy/wonder-blocks-tokens": minor | |||
There was a problem hiding this comment.
Should these be majors as some of the font-sizes are changing? Or just minor?
There was a problem hiding this comment.
IMO it should be minor if only the values change. If the font structure changes (public API), then that could be considered as a breaking change.
TBH this is the kind of situation were any option could work :)
There was a problem hiding this comment.
Alright, I'll leave it as-is!
There was a problem hiding this comment.
Since we are using sizing tokens now for the font.size and font.lineHeight tokens, the token values will be strings now including the rem unit, instead of the numeric value. Would this be a breaking change since something like fontSize: ${font.size.small}px would need to be updated so it doesn't become font-size: 1.4rempx?
(Or, depending on if the css variable changes are released first, it'd become font-size: var(--wb-token-name)px)
There was a problem hiding this comment.
Great question. That does seem breaking to me as it will have to be updated at the call-sites to prevent an issue. I see numerous instances of px added after font.size and font.lineHeight tokens in webapp!
| fontFamily: font.family.serif, | ||
| fontWeight: font.weight.regular, | ||
| fontSize: font.size.large, | ||
| lineHeight: font.lineHeight.large, |
There was a problem hiding this comment.
Note: Because the font-size was changed to 20px for BodySerifBlock, I updated the line height to match the scale (24px).
| lineHeight: "22px", | ||
| fontFamily: font.family.mono, | ||
| fontWeight: font.weight.regular, | ||
| fontSize: font.size.medium, |
There was a problem hiding this comment.
Note: 17px isn't on our REM scale. Rather than use a one-off value, I converted BodyMonospace to font.size.medium (16px).
| fontFamily: font.family.mono, | ||
| fontWeight: font.weight.regular, | ||
| fontSize: font.size.medium, | ||
| lineHeight: font.lineHeight.medium, |
There was a problem hiding this comment.
Note: 22px also isn't on our REM scale so I converted BodyMonospace to font.lineHeight.medium (20px)
For Font-size: large or extra-large, the -64px margin on `<main>` was cutting off the h1 on the overview page. I decreased the top padding instead, as an experiment. We could add padding back in to other pages, if they need it.
…REMs instead of pixels for user-scalable font-sizing and line-heights.
Also exports utility functions for converting between Pixels and REMs
… scale for scalable font-sizing
…font tokens with REM units instead of pixels for user-scalable font-sizing and line-heights.
…rnally for scalable font-sizes
marcysutton
force-pushed
the
FEI-4065-rems
branch
from
5d73aa7 to
506d07e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2532 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary:
Update wonder-blocks-typography component styles to use REMs for font-sizing and line-heights. This PR follows #2526 where the base font size was changed to 62.5%.
Issue: WB-1927, FEI-6534
Test plan:
BodySerifBlocknow has a line-height of24px(it was28pxbefore)BodySerifnow has a font-size of16pxand line-height of20pxto leverage t-shirt sizing (it was18px/22pxbefore)BodyMonospacenow has a font-size of16pxand line-height of20px(it was17px/22pxbefore)TextAreamin-height andButton-Coreline-heights are computed with actual valuesAdjustments for REM sizing and reflow in webapp are covered under individual font-sizing issues: https://khanacademy.atlassian.net/issues/?jql=labels%20%3D%20%22webapp-font-sizing-rems%22