[WB-1814.7] Refactor Modal to use semantic colors #2468

jandrade · 2025-02-10T17:08:46Z

Summary:

Next step is to refactor the Modal package to use semantic colors.

Besides the migration, this PR also includes the following changes:

Reworked the theme structure to split tokens by sub-component.
Updated stories to use the new semantic colors.
Adapted some components to use theming instead of hardcoded colors.

Implementation plan:

[WB-1814.1] Refactor Checkbox and Radio to use semantic colors #2439
[WB-1814.2] Refactor TextField and TextArea to use semantic colors #2440
[WB-1814.3] Refactor Switch to use semantic colors #2441
[WB-1814.4] Refactor Accordion, Banner, BirthdayPicker to use semantic colors #2446
[WB-1814.5] Refactor IconButton to use semantic colors #2449
[WB-1814.6] Refactor Link to use semantic colors #2464
Modal (current PR)
Popover, Tooltip
Dropdown
Clickable, Pill, Toolbar

Issue: WB-1814

Test plan:

Verify that the Modal Chromatic snapshots are mostly unchanged.

URL: /?path=/docs/packages-modal-onepanedialog--docs&globals=viewport:desktop

…ticColors. Restructure theme contract.

changeset-bot · 2025-02-10T17:08:50Z

🦋 Changeset detected

Latest commit: 473de27

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages

Name	Type
@khanacademy/wonder-blocks-theming	Minor
@khanacademy/wonder-blocks-modal	Patch
@khanacademy/wonder-blocks-button	Patch
@khanacademy/wonder-blocks-icon-button	Patch
@khanacademy/wonder-blocks-switch	Patch
@khanacademy/wonder-blocks-dropdown	Patch
@khanacademy/wonder-blocks-popover	Patch
@khanacademy/wonder-blocks-tooltip	Patch
@khanacademy/wonder-blocks-banner	Patch
@khanacademy/wonder-blocks-search-field	Patch
@khanacademy/wonder-blocks-birthday-picker	Patch

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

github-actions · 2025-02-10T17:09:40Z

Size Change: +81 B (+0.08%)

Total Size: 97.6 kB

Filename	Size	Change
`packages/wonder-blocks-modal/dist/es/index.js`	5.5 kB	+81 B (+1.5%)

ℹ️ View Unchanged

Filename	Size
`packages/wonder-blocks-accordion/dist/es/index.js`	3.53 kB
`packages/wonder-blocks-banner/dist/es/index.js`	1.55 kB
`packages/wonder-blocks-birthday-picker/dist/es/index.js`	1.82 kB
`packages/wonder-blocks-breadcrumbs/dist/es/index.js`	886 B
`packages/wonder-blocks-button/dist/es/index.js`	4.3 kB
`packages/wonder-blocks-cell/dist/es/index.js`	2.01 kB
`packages/wonder-blocks-clickable/dist/es/index.js`	3.04 kB
`packages/wonder-blocks-core/dist/es/index.js`	2.97 kB
`packages/wonder-blocks-data/dist/es/index.js`	6.25 kB
`packages/wonder-blocks-dropdown/dist/es/index.js`	19 kB
`packages/wonder-blocks-form/dist/es/index.js`	6.03 kB
`packages/wonder-blocks-grid/dist/es/index.js`	1.36 kB
`packages/wonder-blocks-icon-button/dist/es/index.js`	3.11 kB
`packages/wonder-blocks-icon/dist/es/index.js`	873 B
`packages/wonder-blocks-labeled-field/dist/es/index.js`	1.22 kB
`packages/wonder-blocks-layout/dist/es/index.js`	1.82 kB
`packages/wonder-blocks-link/dist/es/index.js`	2.27 kB
`packages/wonder-blocks-pill/dist/es/index.js`	1.4 kB
`packages/wonder-blocks-popover/dist/es/index.js`	4.86 kB
`packages/wonder-blocks-progress-spinner/dist/es/index.js`	1.52 kB
`packages/wonder-blocks-search-field/dist/es/index.js`	1.33 kB
`packages/wonder-blocks-switch/dist/es/index.js`	2 kB
`packages/wonder-blocks-testing-core/dist/es/index.js`	3.73 kB
`packages/wonder-blocks-testing/dist/es/index.js`	1.07 kB
`packages/wonder-blocks-theming/dist/es/index.js`	679 B
`packages/wonder-blocks-timing/dist/es/index.js`	1.79 kB
`packages/wonder-blocks-tokens/dist/es/index.js`	2.51 kB
`packages/wonder-blocks-toolbar/dist/es/index.js`	905 B
`packages/wonder-blocks-tooltip/dist/es/index.js`	6.99 kB
`packages/wonder-blocks-typography/dist/es/index.js`	1.23 kB

_{compressed-size-action}

github-actions · 2025-02-10T17:14:50Z

A new build was pushed to Chromatic! 🚀

https://5e1bf4b385e3fb0020b7073c-eyjmktobvn.chromatic.com/

Chromatic results:

Metric	Total
Captured snapshots	180
Tests with visual changes	0
Total stories	543
Inherited (not captured) snapshots [TurboSnap]	200
Tests on the build	380

… withScopedTheme HOC.

…- shadow)

jandrade · 2025-02-10T17:35:16Z

packages/wonder-blocks-modal/src/components/modal-backdrop.tsx

note: this file didn't support theming yet, so I used the HOC available for that.

jandrade · 2025-02-10T17:35:37Z

packages/wonder-blocks-modal/src/components/modal-footer.tsx

note: Also applied theming here, but using the hook in this case.

It's great we have the option to apply theming both ways, nice job 😄

jandrade · 2025-02-10T17:37:10Z

packages/wonder-blocks-modal/src/themes/default.ts

note: Restructured the theme contract to split it by component, then separate tokens by semantic category:

e.g.

// theme.component.category.property theme.header.color.background theme.footer.spacing.gap

jandrade · 2025-02-10T17:38:27Z

packages/wonder-blocks-theming/src/index.ts

@@ -5,6 +5,7 @@ export {default as useScopedTheme} from "./hooks/use-scoped-theme";
 export {
    default as withScopedTheme,
    type WithThemeProps,
+    type WithoutTheme,


note: I had to expose this in order to use withScopedTheme in other WB packages, otherwise TS would complain b/c it needed that type for inferring the correct types in the HOC consumer (ModalBackdrop in this case).

khan-actions-bot · 2025-02-10T17:38:51Z

Gerald

Required Reviewers

@Khan/wonder-blocks for changes to .changeset/nice-maps-tap.md, .changeset/odd-fans-provide.md, __docs__/wonder-blocks-modal/modal-dialog.stories.tsx, __docs__/wonder-blocks-modal/modal-panel.stories.tsx, __docs__/wonder-blocks-modal/one-pane-dialog.stories.tsx, packages/wonder-blocks-theming/src/index.ts, packages/wonder-blocks-modal/src/components/modal-backdrop.tsx, packages/wonder-blocks-modal/src/components/modal-dialog.tsx, packages/wonder-blocks-modal/src/components/modal-footer.tsx, packages/wonder-blocks-modal/src/components/modal-header.tsx, packages/wonder-blocks-modal/src/components/modal-panel.tsx, packages/wonder-blocks-modal/src/themes/default.ts, packages/wonder-blocks-modal/src/themes/khanmigo.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2025-02-10T17:39:45Z

npm Snapshot: Published

🎉 Good news!! We've packaged up the latest commit from this PR (44b6399) 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="PR2468"

Packages can also be installed manually by running:

pnpm add @khanacademy/wonder-blocks-<package-name>@PR2468

beaesguerra

Looks good, left some non-blocking questions!

beaesguerra · 2025-02-11T00:14:54Z

packages/wonder-blocks-modal/src/components/modal-footer.tsx

It's great we have the option to apply theming both ways, nice job 😄

beaesguerra · 2025-02-11T00:16:10Z

packages/wonder-blocks-modal/src/components/modal-header.tsx

@@ -153,41 +153,40 @@ const small = "@media (max-width: 767px)";

 const themedStylesFn: ThemedStylesFn<ModalDialogThemeContract> = (theme) => ({
    header: {
-        boxShadow: `0px 1px 0px ${theme.color.shadow.default}`,
+        boxShadow: `0px 1px 0px ${theme.header.color.border}`,


(for later): Do you think box shadow values should belong in the theme?

Yes, but I'm planning to address first global elevation tokens in a separate ticket. I'll add a TODO comment here.

beaesguerra · 2025-02-11T00:19:28Z

packages/wonder-blocks-modal/src/components/modal-panel.tsx

-            outlineWidth: theme.border.width,
-            outlineColor: theme.border.color,
+            outlineWidth: theme.root.border.width,
+            outlineColor: theme.panel.color.border,


Would it make sense for this to be theme.root.color.border? Or for the outlineWidth/borderRadius to be theme.panel.border.width and theme.panel.border.radius so using root or panel styles is consistent?

I originally used theme.panel.border.* but noticed that this value is shared with the Dialog component too, and that's where I moved it to root. The border color token is specific to panel in this case, and that's why I moved it here. I'll move it to the root object to make it more consistent.

beaesguerra · 2025-02-11T00:22:53Z

packages/wonder-blocks-modal/src/themes/default.ts

+     * Building blocks
+     */
+    backdrop: {
+        color: {


For the theme component tokens with building blocks, do you think it's helpful to keep the additional separation between color,spacing, etc? Curious what you think!

I still see benefit in terms of readability and consistency. Also portability in case we need to refactor or move stuff around.

beaesguerra · 2025-02-11T00:33:13Z

packages/wonder-blocks-modal/src/themes/default.ts

+            border: semanticColor.border.focus,
+        },
+        spacing: {
+            paddingBlockEnd: spacing.xLarge_32,


For token names, what do you think about naming the tokens so that they aren't specific to how it is used in css? That way, if we were to change how we apply the spacing value in the component, we wouldn't have to rename the token!

I think it makes sense in this case as it can be renamed to gap (not directly related to the css prop). In other cases, it might be trickier where tokens are directly associated to specific properties (e.g. the header padding).

beaesguerra · 2025-02-11T00:38:13Z

packages/wonder-blocks-modal/src/themes/default.ts

+    root: {
+        // TODO(WB-1852): Remove light variant.
+        color: {
+            inverse: {


Is there a reason we don't need the background and foreground defined for the default case (non-inverse)?

No reason at all, but I don't see why we shouldn't have that.

marcysutton

Left you a few additional questions! Thanks for doing all of this theming work, @jandrade!

marcysutton · 2025-02-11T05:45:54Z

packages/wonder-blocks-modal/src/components/modal-header.tsx

        display: "flex",
        flexDirection: "column",
        minHeight: 66,
-        padding: `${theme.spacing.header.medium}px ${theme.spacing.header.large}px`,
+        padding: `${theme.header.spacing.paddingBlockMd}px ${theme.header.spacing.paddingInlineMd}px`,


Could we put the px unit into a variable so it could be easily replaced later (for rem or em)?

marcysutton · 2025-02-11T05:47:08Z

packages/wonder-blocks-modal/src/components/modal-header.tsx

        [small]: {
-            paddingRight: theme.spacing.header.large,
+            paddingRight: theme.header.spacing.titlePaddingRightSm,


The padding appears to have gone from large to sm, is that intentional?

marcysutton · 2025-02-11T05:48:01Z

packages/wonder-blocks-modal/src/themes/default.ts

+            paddingInlineSm: spacing.medium_16,
+            gap: spacing.xSmall_8,
+            titlePaddingRightMd: spacing.medium_16,
+            titlePaddingRightSm: spacing.xLarge_32,


Should this be titlePaddingRightXl?

marcysutton · 2025-02-11T05:49:29Z

packages/wonder-blocks-modal/src/themes/default.ts

+            titlePaddingRightSm: spacing.xLarge_32,
+        },
+    },
+    panel: {


I like how you grouped these styles by component!

codecov · 2025-02-20T18:20:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (d9c3fae) to head (473de27).
Report is 8 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff      @@
##   main   #2468   +/-   ##
============================
============================

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9c3fae...473de27. Read the comment docs.

…#2470) ## Summary: Next step is to refactor the `Popover`, `Tooltip` and `Toolbar` packages to use semantic colors. ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. #2464 7. #2468 8. Popover, Tooltip, Toolbar (current PR) 9. Dropdown 10. Clickable, Pill Issue: WB-1814 ## Test plan: Verify that the Modal Chromatic snapshots are mostly unchanged. Author: jandrade Reviewers: beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2470

## Summary: - Migrate Clickable and Pill to use semantic colors - Add `All Variants` story to Pill ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. #2464 7. #2468 8. #2470 9. Clickable, Pill (current PR) 10. Dropdown Issue: WB-1814 ## Test plan: Navigate to the `Clickable` and `Pill` stories in Storybook and verify that the colors are correct. Author: jandrade Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ gerald, ⏭️ dependabot Pull Request URL: #2472

…2474) ## Summary: Last PR that migrates the remaining components to use semantic colors. - Added `All Variants` stories to `ActionItem` and `OptionItem`. - Refactored `ActionItem` and `OptionItem` to use semantic colors. - Also refactored `ActionMenu`, `SingleSelect`, `MultiSelect` and `Combobox` to use semantic colors. - Simplified the styles in `ActionMenuOpenerCore` to use a single static StyleSheet instance (instead of generating another one in runtime). ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. #2464 7. #2468 8. #2470 9. #2472 10. Dropdown (current PR) Issue: WB-1814 ## Test plan: Verify that all the dropdown stories are still working as expected. Author: jandrade Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ dependabot, ✅ gerald Pull Request URL: #2474

Juan Andrade added 2 commits February 10, 2025 12:02

[semantic-modal] [WB-1814.7] Refactor Modal to use semantic colors

56cf715

[semantic-modal] docs(changeset): Refactor Modal package to use seman…

5609c18

…ticColors. Restructure theme contract.

jandrade self-assigned this Feb 10, 2025

Juan Andrade added 2 commits February 10, 2025 12:17

[semantic-modal] docs(changeset): Expose WithoutTheme type for use in…

c1fdfd2

… withScopedTheme HOC.

[semantic-modal] Fix token to use correct value (header border color …

31798bc

…- shadow)

jandrade commented Feb 10, 2025

View reviewed changes

jandrade marked this pull request as ready for review February 10, 2025 17:38

khan-actions-bot requested a review from a team February 10, 2025 17:38

beaesguerra approved these changes Feb 11, 2025

View reviewed changes

marcysutton reviewed Feb 11, 2025

View reviewed changes

jandrade mentioned this pull request Feb 11, 2025

[WB-1814.8] Refactor Popover, Tooltip, Toolbar to use semantic colors #2470

Merged

[semantic-modal] Address feedback

473de27

This was referenced Feb 12, 2025

[WB-1814.9] Refactor Clickable and Pill to use semantic colors #2472

Merged

[WB-1814.10] Refactor wonder-blocks-dropdown to use semantic colors #2474

Merged

jandrade merged commit 6d37702 into main Feb 20, 2025
14 checks passed

jandrade deleted the semantic-modal branch February 20, 2025 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WB-1814.7] Refactor Modal to use semantic colors #2468

[WB-1814.7] Refactor Modal to use semantic colors #2468

jandrade commented Feb 10, 2025

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

jandrade Feb 10, 2025

jandrade Feb 10, 2025

beaesguerra Feb 11, 2025

jandrade Feb 10, 2025

jandrade Feb 10, 2025

khan-actions-bot commented Feb 10, 2025

github-actions bot commented Feb 10, 2025 •

edited

Loading

beaesguerra left a comment

beaesguerra Feb 11, 2025

beaesguerra Feb 11, 2025

jandrade Feb 11, 2025

beaesguerra Feb 11, 2025

jandrade Feb 11, 2025

beaesguerra Feb 11, 2025

jandrade Feb 11, 2025

beaesguerra Feb 11, 2025

jandrade Feb 11, 2025

beaesguerra Feb 11, 2025

jandrade Feb 11, 2025

marcysutton left a comment

marcysutton Feb 11, 2025

marcysutton Feb 11, 2025

marcysutton Feb 11, 2025

marcysutton Feb 11, 2025

codecov bot commented Feb 20, 2025

[WB-1814.7] Refactor Modal to use semantic colors #2468

[WB-1814.7] Refactor Modal to use semantic colors #2468

Conversation

jandrade commented Feb 10, 2025

Summary:

Implementation plan:

Test plan:

changeset-bot bot commented Feb 10, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 10, 2025 • edited Loading

github-actions bot commented Feb 10, 2025 • edited Loading

A new build was pushed to Chromatic! 🚀

Chromatic results:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khan-actions-bot commented Feb 10, 2025

Gerald

github-actions bot commented Feb 10, 2025 • edited Loading

npm Snapshot: Published

beaesguerra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2025

Codecov Report

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading