-
Notifications
You must be signed in to change notification settings - Fork 10
[WB-1914] Update wonder-blocks-tokens to use Base 10 #2526
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
Conversation
🦋 Changeset detectedLatest commit: c08daa8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 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 |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (99a9db1) 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="PR2526" Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2526 |
Size Change: +20 B (+0.02%) Total Size: 103 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-ynihtbkgiu.chromatic.com/ Chromatic results:
|
@@ -2,7 +2,7 @@ | |||
* Overrides for the Storybook preview iframe. | |||
*/ | |||
html { | |||
font-size: 50%; | |||
font-size: 62.5%; |
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.
This change isn't registering in Chromatic yet! It still shows 50% and a bunch of stuff looks off. Weird! https://5e1bf4b385e3fb0020b7073c-jxifgupglv.chromatic.com/sb-styles/preview.css
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.
I'm seeing it as 62.5% now! https://5e1bf4b385e3fb0020b7073c-ohuxdtgckx.chromatic.com/?path=/story/packages-banner--default
The typography in the docs are quite large though, is this expected? My zoom is at 100%! https://5e1bf4b385e3fb0020b7073c-ohuxdtgckx.chromatic.com/?path=/docs/overview--docs
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.
Ah yes, good catch! I fiddled with those numbers some more and it's looking better to me. There is a slight shift on the overview page, possibly from a dynamic width or height somewhere. I can look into it if it's important to address..it seemed kindof trivial though!
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.
It's really odd that Chromatic is not using the last generated storybook instance. I can also see what @marcysutton mentions.
- Chromatic is using this hash to run snapshots:
jxifgupglv
. - The last site generated and uploaded to Chromatic is
jnmmnedmeq
.
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.
It's looking correct to me now on the Chromatic URL here in the PR: https://5e1bf4b385e3fb0020b7073c-jnmmnedmeq.chromatic.com/?path=/docs/overview--docs
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.
Agreed, I understand that (you can see the IDs/hashes included in the comment).
My worryness with this is that Chromatic is using the wrong snapshots for visual regression testing and this could be a possible caused for flaky-ness in the future.
Have you tried pushing more changes to see if the build for visual tests changes?
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.
Good question! I just pushed another update to override some Storybook styling, that depended on Base 8 font size. I'm not seeing the same layout shifts anymore, so we'll see if it helps the visual tests!
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.
YAY it works again, thanks for trying it out again :)
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.
Thanks for making these updates Marcy! Left a comment around the typography sizing in the docs!
Also, I think we'll need to update the sizing token docs and spacing to sizing mapping to reflect the new tokens! https://khan.github.io/wonder-blocks/?path=/docs/packages-tokens-sizing--docs
@@ -2,7 +2,7 @@ | |||
* Overrides for the Storybook preview iframe. | |||
*/ | |||
html { | |||
font-size: 50%; | |||
font-size: 62.5%; |
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.
I'm seeing it as 62.5% now! https://5e1bf4b385e3fb0020b7073c-ohuxdtgckx.chromatic.com/?path=/story/packages-banner--default
The typography in the docs are quite large though, is this expected? My zoom is at 100%! https://5e1bf4b385e3fb0020b7073c-ohuxdtgckx.chromatic.com/?path=/docs/overview--docs
…e Base 10 rem units instead of Base 8
8e7f15c
to
d6fe326
Compare
d6fe326
to
5b837c5
Compare
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.
Now that the base font size in Chromatic is up to date, I noticed a few more uses of sizing tokens that need to be updated!
}, | ||
description: { | ||
color: semanticColor.text.secondary, | ||
paddingBlockEnd: sizing.size_150, | ||
paddingBlockEnd: sizing.size_120, | ||
}, | ||
errorSection: { | ||
flexDirection: "row", | ||
gap: sizing.size_100, |
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.
The gap will also need to be updated to sizing.size_080
for 8px! (figma)

Noticed this change in the Chromatic snapshot! I was expecting there'd be no visual changes since things would be converted to the new sizing!
@@ -235,7 +235,7 @@ const styles = StyleSheet.create({ | |||
}, | |||
list: { | |||
// Add horizontal padding for focus outline of first/last elements | |||
paddingInline: sizing.size_050, | |||
paddingInline: sizing.size_040, |
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.
The gap will also need to be updated to sizing.size_160
! Noticed it by comparing the stories from the snapshot diff!
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.
Good call! I found a bunch of similar tokens. I was about to dig in to see what was causing the diffs, hopefully these fix them all!
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.
Woot, I believe we are clear of diff changes now!
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.
Looks good, nice work! 🎉 I'm glad we have lots of Chromatic snapshots to help verify our changes 😄
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.
Looks great! thanks for updating all of this 🎉 🚀
@beaesguerra whew, I agree! Chromatic is SO helpful! Thanks for the approvals. I'm putting this into a |
e2b5331
to
c08daa8
Compare
Now that I have a PR open for updating sizing tokens in webapp (https://github.com/Khan/webapp/pull/31173), I think we should release these updates so that PR can be updated with an actual release! #2538 The next step after this will be to update the |
…to `main` (#2538) ## Summary: This PR includes the following commits: - [WB-1914] Update wonder-blocks-tokens to use Base 10 (#2526) Issue: WB-1914 ## Test plan: 1. Reference Base 10 scale in https://khanacademy.atlassian.net/wiki/spaces/WB/pages/3932520659/Sizing+Conversion+Table+for+devs 2. Double-check math of values used 3. Ensure there are no lint errors with tokens used in WB 4. Ensure there are no Chromatic changes 5. Complete updates to usage in webapp (PR: Khan/webapp#31173) [WB-1914]: https://khanacademy.atlassian.net/browse/WB-1914?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Author: marcysutton Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: jandrade, beaesguerra Checks: ✅ 17 checks were successful, ⏭️ 3 checks have been skipped, ⏹️ 8 checks were cancelled Pull Request URL: #2538
Summary:
This change updates the sizing tokens in wonder-blocks-tokens to use Base 10 rather than Base 8. We decided on this change as a team to work around some minimum issues in browsers where JavaScript returned the incorrect font-size on a page.
Issue: WB-1914
Test plan: