[WB-1867.3] Add ThunderBlocks theme to modal #2678
Conversation
🦋 Changeset detectedLatest commit: e91690b The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 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 |
|
Size Change: +47 B (+0.05%) Total Size: 101 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-houzbkkdqz.chromatic.com/ Chromatic results:
|
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 (a20ac8f) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2678"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2678 |
| @@ -132,6 +133,13 @@ const withThemeSwitcher: Decorator = ( | |||
| Story, | |||
| {globals: {theme}, parameters: {enableRenderStateRootDecorator}}, | |||
| ) => { | |||
| React.useEffect(() => { | |||
| if (theme) { | |||
There was a problem hiding this comment.
Will we be removing the ThemeSwitcher related things later on? The components that were for the original theming mechanism!
There was a problem hiding this comment.
I don't think so. We'll need it just in case someone needs to override the theme down in the tree for any reason.
| @@ -119,6 +119,7 @@ const core = { | |||
| }, | |||
| shadow: { | |||
| transparent: transparent, | |||
| instructive: `color-mix(in srgb, ${color.blue_10} 20%, ${transparent})`, | |||
There was a problem hiding this comment.
Will this token also be added in Figma? The color tokens in the handshake file doesn't include this token! Though it seems the transparent token for TB has a blueish tint?
There was a problem hiding this comment.
oh good catch! turns out that I was using the incorrect tokens and incorrect values. I fixed that by moving it to core.shadow.transparent, and using the correct value in the modal component-level dialog.shadow.default token. It should now match the designs as expected. Let me know what you think.
## Summary: - Removes the `khanmigo` theme from modal in favor of CSS vars. - Uses `sizing` instead of `spacing`. ### Implementation plan 1. #2676 2. #2677 3. #2678 Issue: https://khanacademy.atlassian.net/browse/WB-1867 ## Test plan: Navigate to the Modal stories in Storybook and ensure that the modals render correctly. Author: jandrade Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ 39 checks were successful, ⏭️ 5 checks have been skipped Pull Request URL: #2676
## Summary: Removes the `light` prop from modal as it is no longer needed. ### Implementation plan 1. #2676 2. #2677 3. #2678 Issue: https://khanacademy.atlassian.net/browse/WB-1867 ## Test plan: Navigate to the Modal stories in Storybook and ensure that the modals render correctly. Verify that the "Dark" stories are no longer present in the docs. Author: jandrade Reviewers: beaesguerra, jandrade Required Reviewers: Approved By: beaesguerra Checks: ✅ 12 checks were successful, ⏭️ 2 checks have been skipped Pull Request URL: #2677
…. Updates typography to use BodyText and Heading. Removes Strut uses.
| <Heading size="xxlarge" id="modal-title-0"> | ||
| Modal Title | ||
| </Heading> | ||
| <BodyText id="modal-desc-0"> |
There was a problem hiding this comment.
Such a clean switchover! Love to see it!
| <br /> | ||
| <Body> | ||
| </BodyText> | ||
| <BodyText> |
There was a problem hiding this comment.
Should we add any margins or a gap here to replace the <br /> tags?
There was a problem hiding this comment.
that's being added to the parent container (in all the stories).
| @@ -118,7 +118,7 @@ const core = { | |||
| }, | |||
| }, | |||
| shadow: { | |||
| transparent: transparent, | |||
| transparent: `color-mix(in srgb, ${color.blue_10} 20%, ${transparent})`, | |||
There was a problem hiding this comment.
What does this line do? Could you add a comment?
There was a problem hiding this comment.
+1! I'm still not sure when we should use color utilities in code 😄
I see in Figma it's set to #252368 20%, though color.blue_10 is #363498
There was a problem hiding this comment.
ohhh it should be color_blue_05 instead.
color-mix is the official CSS way of generating alpha colors. I couldn't use the WB fade() function as it doesn't work with CSS variables (same happens when concatenating WB tokens with the alpha channel).
BEFORE:
const fadedBlue20 = fade(color.blue, 0.2) // e.g. fade(#00f, 0.2) = rgba(0, 0, 255, 0.2)
This doesn't work anymore because the semantic color variable gets converted to a reference to a CSS variable, so we have to use CSS functions as a workaround.
I'll add a note there.
There was a problem hiding this comment.
Added a comment explaining this
.changeset/gold-yaks-grab.md
Outdated
| "@khanacademy/wonder-blocks-tokens": minor | ||
| --- | ||
|
|
||
| Add core.shadow token. Export modal tokens. |
There was a problem hiding this comment.
Do we need to update the changeset since we don't add a new shadow token anymore?
There was a problem hiding this comment.
This was to make sure that I would include the CSS vars generated for WB Modal for all the 3 stacked PRs. But turns out that I didn't land this last PR with the other two, so I messed up the release (and probably this is not needed anymore).
There was a problem hiding this comment.
If we keep the changeset, could we remove the Add core.shadow token part since we no longer add the instructive shadow token?
| @@ -118,7 +118,7 @@ const core = { | |||
| }, | |||
| }, | |||
| shadow: { | |||
| transparent: transparent, | |||
| transparent: `color-mix(in srgb, ${color.blue_10} 20%, ${transparent})`, | |||
There was a problem hiding this comment.
+1! I'm still not sure when we should use color utilities in code 😄
I see in Figma it's set to #252368 20%, though color.blue_10 is #363498
.changeset/gold-yaks-grab.md
Outdated
| "@khanacademy/wonder-blocks-tokens": minor | ||
| --- | ||
|
|
||
| Add core.shadow token. Export modal tokens. |
There was a problem hiding this comment.
If we keep the changeset, could we remove the Add core.shadow token part since we no longer add the instructive shadow token?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2678 +/- ##
======================================
Coverage ? 0
======================================
Files ? 0
Lines ? 0
Branches ? 0
======================================
Hits ? 0
Misses ? 0
Partials ? 0 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary:
Last PR to add Thunderblocks theme to modal components.
Figma: https://www.figma.com/design/EuFu0U7gqc1koXm8ZhlOLp/%E2%9A%A1%EF%B8%8F-Components?node-id=2213-141&m=dev
Implementation plan
lightprop from Modal package. #2677Issue: https://khanacademy.atlassian.net/browse/WB-1867
Test plan:
Navigate to the Modal stories in Storybook and ensure that the modals render
correctly.
Make sure that the typography elements are looking as expected.