Theme: Add density support for surface padding #73215
Conversation
aduth
added
[Type] Enhancement
|
Size Change: +555 B (+0.02%) Total Size: 2.51 MB
ℹ️ View Unchanged
|
|
Cool :) Appreciate it's a separate thing but it'd be neat if it was possible to change theme aspects like density in the Storybook toolbar. |
| [data-wpds-theme-provider-id][data-wpds-density='compact'] { | ||
| --wpds-dimension-padding-surface-large: 16px; /* Large spacing for surfaces */ | ||
| --wpds-dimension-padding-surface-medium: 12px; /* Medium spacing for surfaces */ | ||
| --wpds-dimension-padding-surface-small: 8px; /* Small spacing for surfaces */ | ||
| --wpds-dimension-padding-surface-x-small: 4px; /* Extra small spacing for surfaces */ | ||
| } | ||
|
|
||
| [data-wpds-theme-provider-id][data-wpds-density='comfortable'] { | ||
| --wpds-dimension-padding-surface-large: 24px; /* Large spacing for surfaces */ | ||
| --wpds-dimension-padding-surface-medium: 20px; /* Medium spacing for surfaces */ | ||
| --wpds-dimension-padding-surface-small: 16px; /* Small spacing for surfaces */ | ||
| --wpds-dimension-padding-surface-x-small: 12px; /* Extra small spacing for surfaces */ | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't have a strong opinion but I'm wondering if a naming convention like xs, sm, md, lg, xl would be better.
There was a problem hiding this comment.
I think we could still change it at this point if we think it'd be an improvement. For context, these are largely carried over from existing conventions like what exist in base-styles (example)
There was a problem hiding this comment.
I think the shorter names are a bit easier to interpret when the scale grows. IE 4xs is slightly more intuitive than 4-x-small. It also makes the tokens names shorter, and a bit easier to scan in a list. So I think I lean in that direction, but only slightly. Keen to hear other thoughts cc @mattmiklic.
packages/theme/src/types.ts
Outdated
| /** | ||
| * The density of the theme. | ||
| */ | ||
| density?: 'compact' | 'comfortable'; |
There was a problem hiding this comment.
Should there be a middle-ground default, 'balanced' or similar?
There was a problem hiding this comment.
Ah, I understand there is a default, but it's not named. Feel free to resolve :D
There was a problem hiding this comment.
Yeah, the absence of an override is considered the default. We could have something more explicit, though I think this mirrors nicely to the way it works in the tokens, which is that the densities are "extensions" (i.e. overrides) to some base value.
There was a problem hiding this comment.
I think we should at least mention in the prop description that it's going to use some other default density as the default. As it looks to a consumer now, we might as well just forgotten to document whether the default is compact or comfortable.
| export const Compact: Story = { | ||
| ...Default, | ||
| globals: { | ||
| density: 'compact', |
There was a problem hiding this comment.
I guess it could be useful to have a comfortable story too. Maybe also a story that includes all densities so you can try out the full matrix of padding/density combos.
There was a problem hiding this comment.
Yeah, and to your other point about Storybook configurability, this is something I had been intentionally punting, though is starting to make it difficult to demonstrate or test the differences, so might make sense to do sooner than later (i.e. maybe implement as part of this work).
| "comfortable": "24px" | ||
| }, | ||
| "description": "Large spacing for surfaces" | ||
| }, |
There was a problem hiding this comment.
I think we'll probably need tokens for 32px and 48px padding. 32px is currently used in Dialogs and 48px in DataViews.
I guess that also means we'll need a new primitive so that 48px can jump to 56px in comfortable density.
I updated in 55afefb to add some global theme configuration to the Storybook toolbar. It ended up being a bit more involved than I initially anticipated (implemented as a custom Storybook addon), but the end result is quite nice. And the toolbar item is only shown on stories in the new "Design System" section. Screen.Recording.2025-11-13.at.3.26.04.PM.mov |
|
I'm going to mark this as ready for review, with a couple notes:
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Awesome!
Do you mean map the tokens to real UI for a visual demo? If so, do you think it would be possible to update the Figma plugin that Marco created to import the spacing tokens? I see they're included in Somewhat related, I do think we'll want to support |
What I had in mind was similar to #72984 (comment), if you had ideas for which primitives values should be referenced by each specific density + spacing token combination. Regarding Figma, I think it's worth revisiting though I'm also holding my breath that they ship their native support soon and it can handle this all better.
This is supported in the component implementation itself and not in the design tokens. Do you think it needs to be in the tokens? i.e. <Box padding={ { block: 'small', inline: 'medium' } } /> |
|
Alternatively, maybe we can make a quick pass at tuning up the names and values of the small, medium, large, etc. tokens as a separate task after merging this one, especially as we'll want to invite some other feedback to those choices that ideally shouldn't hold up this one. |
I had a chat with my AI companion today and I think we may end up needing block vs. inline spacing as part of tokens for specific groupings of tokens where it makes sense to encode those design decisions (e.g. inputs / "controls", etc.) but not necessarily as baseline for all spacing tokens. Or this could be a component-level consideration, like inputs defining their inline padding as .input {
padding-block: var(--wpds-dimension-padding-control-sm);
padding-inline: calc(var(--wpds-dimension-padding-control-sm) * 2);
} |
package.json
Outdated
| "storybook:build": "storybook build -c ./storybook -o ./storybook/build", | ||
| "storybook:dev": "concurrently \"npm run dev\" \"wait-on .dev-ready && storybook dev -c ./storybook -p 50240\"", | ||
| "storybook:e2e:dev": "concurrently \"npm run dev\" \"wait-on .dev-ready && storybook dev -c test/storybook-playwright/storybook -p 50241\"", | ||
| "storybook:dev": "npm run build && storybook dev -c ./storybook -p 50240", |
There was a problem hiding this comment.
- As far as I can tell, we don't need to be watching files manually, and can entrust that to Storybook? Maybe I'm missing something, but I tested live updates to components and it seemed to work fine.
If the basic setup hasn't changed, normal watchers like in the Storybook dev script won't be able to auto-rebuild changes in the Sass stylesheets in most wp packages. All the stylesheets are just imported in the index stylesheet and not imported by the component files that use them, so there isn't a proper dependency graph for a standard dev script to follow.
There was a problem hiding this comment.
Okay 👍 The current script here is busted though, because it writes .dev-ready before the build even finishes.
Here's some ideas:
- We could somehow shift the
.dev-readyinto the package, or expect it to provide some signal to the script when it's appropriate to write the file - Similar to something I suggested earlier, we could do an initial build and then switch to "watch" for the Storybook. We'd probably want to add some support to
devto "skip initial build", also because we don't want it to clean (read: destroy) the built files
There was a problem hiding this comment.
Some of this could happen in a follow-up. I think the current state is much more broken than not having stylesheets recompiled on changes (a "nice to have").
There was a problem hiding this comment.
Weird, I just tried npm run storybook: dev on trunk and it's largely working for me (Storybook boots without error), aside from these warnings:
[1] WARN export 'default' (imported as 'FontSize') was not found in './font-sizes/font-size' (module has no exports)
[1] WARN Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN Deprecation Warning on line 0, column 8 of file:///Users/lena/Documents/Work/Automattic/gutenberg/storybook/global-basic.lazy.scss:0:8:
[1] WARN Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.
[1] WARN
[1] WARN More info and automated migrator: https://sass-lang.com/d/import
[1] WARN
[1] WARN 0 | @import "~@wordpress/base-styles/variables";
[1] WARN
[1] WARN
[1] WARN ../../../../storybook/global-basic.lazy.scss 1:9 root stylesheet
[1] WARN
[1] WARN Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN Deprecation Warning on line 0, column 8 of file:///Users/lena/Documents/Work/Automattic/gutenberg/storybook/global-wordpress.lazy.scss:0:8:
[1] WARN Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.
[1] WARN
[1] WARN More info and automated migrator: https://sass-lang.com/d/import
[1] WARN
[1] WARN 0 | @import "~@wordpress/base-styles/colors";
[1] WARN
[1] WARN
[1] WARN ../../../../storybook/global-wordpress.lazy.scss 1:9 root stylesheet
[1] WARN
[1] WARN
[1] Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN Deprecation Warning on line 0, column 8 of file:///Users/lena/Documents/Work/Automattic/gutenberg/storybook/stories/playground/fullpage/style.lazy.scss:0:8:
[1] WARN Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.
[1] WARN
[1] WARN More info and automated migrator: https://sass-lang.com/d/import
[1] WARN
[1] WARN 0 | @import "~@wordpress/base-styles/colors";
[1] WARN
[1] WARN
[1] WARN ../../../../storybook/stories/playground/fullpage/style.lazy.scss 1:9 root stylesheet
[1] WARN
[1] WARN Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN Deprecation Warning on line 1, column 8 of file:///Users/lena/Documents/Work/Automattic/gutenberg/storybook/stories/playground/fullpage/style.lazy.scss:1:8:
[1] WARN Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.
[1] WARN
[1] WARN More info and automated migrator: https://sass-lang.com/d/import
[1] WARN
[1] WARN 1 | @import "~@wordpress/base-styles/variables";
[1] WARN
[1] WARN
[1] WARN ../../../../storybook/stories/playground/fullpage/style.lazy.scss 2:9 root stylesheet
[1] WARN
[1] WARN Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN Deprecation Warning on line 2, column 8 of file:///Users/lena/Documents/Work/Automattic/gutenberg/storybook/stories/playground/fullpage/style.lazy.scss:2:8:
[1] WARN Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.
[1] WARN
[1] WARN More info and automated migrator: https://sass-lang.com/d/import
[1] WARN
[1] WARN 2 | @import "~@wordpress/base-styles/mixins";
[1] WARN
[1] WARN
[1] WARN ../../../../storybook/stories/playground/fullpage/style.lazy.scss 3:9 root stylesheet
[1] WARN
[1] WARN Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN Deprecation Warning on line 3, column 8 of file:///Users/lena/Documents/Work/Automattic/gutenberg/storybook/stories/playground/fullpage/style.lazy.scss:3:8:
[1] WARN Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.
[1] WARN
[1] WARN More info and automated migrator: https://sass-lang.com/d/import
[1] WARN
[1] WARN 3 | @import "~@wordpress/base-styles/breakpoints";
[1] WARN
[1] WARN
[1] WARN ../../../../storybook/stories/playground/fullpage/style.lazy.scss 4:9 root stylesheet
[1] WARN
[1] WARN Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN Deprecation Warning on line 4, column 8 of file:///Users/lena/Documents/Work/Automattic/gutenberg/storybook/stories/playground/fullpage/style.lazy.scss:4:8:
[1] WARN Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.
[1] WARN
[1] WARN More info and automated migrator: https://sass-lang.com/d/import
[1] WARN
[1] WARN 4 | @import "~@wordpress/base-styles/animations";
[1] WARN
[1] WARN
[1] WARN ../../../../storybook/stories/playground/fullpage/style.lazy.scss 5:9 root stylesheet
[1] WARN
[1] WARN Module Warning (from ./node_modules/sass-loader/dist/cjs.js):
[1] WARN 2 repetitive deprecation warnings omitted.
[1] WARN Run in verbose mode to see all warnings.
(Not sure what the FontSize problem is, that's new to me.)
Are you seeing any errors that prevent Storybook from booting? I also checked the file history, and I couldn't really identify any changes that would fundamentally break the original wait mechanism. IIRC, we basically only need to wait for the initial tsc --build to complete, and that's still intact.
There was a problem hiding this comment.
My impression is that it's a race condition which may not always be manifesting consistently as an error.
What I can see in trunk which validates the potential for race conditions is that in the output of npm run storybook:dev , you can see transpilation messages intermixed with Storybook output, which is a strong signal that Storybook is not waiting for the build to finish before starting. I believed this was the same sort of issue that prompted your changes in #72487
Example snippet showing transpilation and Storybook book happening concurrently for me on trunk:
[0] ✔ Transpiled eslint-plugin (29ms)
[0] ✔ Transpiled keycodes (136ms)
[0] ✔ Transpiled react-i18n (135ms)
[0] ✔ Transpiled date (139ms)
[0] ✔ Transpiled a11y (139ms)
[1] @storybook/core v8.4.7
[1]
[0] ✔ Transpiled primitives (148ms)
[0] ✔ Transpiled theme (158ms)
[0] ✔ Transpiled api-fetch (159ms)
[0] ✔ Transpiled dom (176ms)
[0] ✔ Transpiled scripts (3ms)
There was a problem hiding this comment.
I see the same, but FWIW that's same behavior as when I first added the wait mechanism. It never waited for transpilation, or anything else in the main build script. If I understand correctly, we don't have to wait for the entire build to complete, just certain "prebuild" that must be ready when the Storybook's builder (webpack) starts.
Yes it's messy, but if the Storybook is working then it's probably fine? 🙈 At least for the scope of this PR.
There was a problem hiding this comment.
Yes it's messy, but if the Storybook is working then it's probably fine? 🙈 At least for the scope of this PR.
It wasn't working originally, which is what prompted the changes.
Although testing again (via git checkout trunk -- bin/dev.mjs .gitignore package.json on this branch), I'm not able to reproduce the errors as the branch exists now.
What I think happened is that in earlier iterations of the pull request, I considered having "fancier" options in the Theme dropdown to show some previews for the color theme using DuotoneSwatch, and it was the import of that component that triggered the error.
I was able to reproduce it by adding back the swatch as a test:
diff --git a/storybook/addons/design-system-theme/register.tsx b/storybook/addons/design-system-theme/register.tsx
index badc085441..9f3396376e 100644
--- a/storybook/addons/design-system-theme/register.tsx
+++ b/storybook/addons/design-system-theme/register.tsx
@@ -11,2 +11,3 @@ import {
} from '@storybook/components';
+import { DuotoneSwatch } from '@wordpress/components';
@@ -83,2 +84,3 @@ const ThemeTool = () => {
<MirrorIcon />
+ <DuotoneSwatch />
Theme
At that point running npm run storybook:dev I see:
[1] ✘ [ERROR] Could not resolve "@wordpress/components"
[1]
[1] storybook/addons/design-system-theme/register.tsx:12:30:
[1] 12 │ import { DuotoneSwatch } from '@wordpress/components';
[1] ╵ ~~~~~~~~~~~~~~~~~~~~~~~
[1]
[1] The module "./build-module/index.js" was not found on the file system:
[1]
[1] node_modules/@wordpress/components/package.json:30:13:
[1] 30 │ "import": "./build-module/index.js",
[1] ╵ ~~~~~~~~~~~~~~~~~~~~~~~~~
[1]
[1] You can mark the path "@wordpress/components" as external to exclude it from the bundle, which will remove this error and leave the unresolved path in the bundle.
[1]
[1] Error: Build failed with 1 error:
[1] storybook/addons/design-system-theme/register.tsx:12:30: ERROR: Could not resolve "@wordpress/components"
For the purpose of this pull request as is and to avoid holding this up, I can revert these changes for now, but I expect it's likely the issue will manifest again in the future.
There was a problem hiding this comment.
I see, yeah that's bad, thanks for posting that repro. I agree we'll need some improvements like you suggested in #73215 (comment), sooner or later.
Sorry, end of a long week... in other words do you mean review For things like inputs (which I'd say is roughly the same as badges) would it make sense to have semantic tokens for them as a family? So like we have |
|
@jameskoster and I were able to connect today and iterate on spacing options in b7c4da6:
|
Now configurable through global theme configuration
aduth
force-pushed
the
add/theme-density
branch
from
b7c4da6 to
79291f9
Compare
|
Flaky tests detected in 4fe0f86. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19516069049
|
packages/theme/src/types.ts
Outdated
| /** | ||
| * The density of the theme. | ||
| */ | ||
| density?: 'compact' | 'comfortable'; |
There was a problem hiding this comment.
I think we should at least mention in the prop description that it's going to use some other default density as the default. As it looks to a consumer now, we might as well just forgotten to document whether the default is compact or comfortable.
Allow resetting if unset density would otherwise inherit from a non-default density ancestor
Avoid interfering with theming shown in ThemeProvider stories
See: #73215 (comment) Co-Authored-By: Lena Morita <[email protected]>
| /** | ||
| * The density of the theme. If left unspecified, the theme inherits from | ||
| * the density of the closest `ThemeProvider`, or uses the default density | ||
| * if there is no inherited density. | ||
| * | ||
| * @default undefined | ||
| */ | ||
| density?: undefined | 'default' | 'compact' | 'comfortable'; |
There was a problem hiding this comment.
Just a thought, no strong opinion, but would something like this be more intuitive? Personally I think it would be easier to understand when skimming the docs, since it's closer to the CSS mental model.
| /** | |
| * The density of the theme. If left unspecified, the theme inherits from | |
| * the density of the closest `ThemeProvider`, or uses the default density | |
| * if there is no inherited density. | |
| * | |
| * @default undefined | |
| */ | |
| density?: undefined | 'default' | 'compact' | 'comfortable'; | |
| /** | |
| * The density of the theme. If left unspecified, the theme inherits from | |
| * the density of the closest `ThemeProvider`, or uses the default density | |
| * if there is no inherited density. | |
| * | |
| * @default 'inherit' | |
| */ | |
| density?: 'inherit' | 'default' | 'compact' | 'comfortable'; |
There was a problem hiding this comment.
Hm, I think there's some advantages to that, particularly around explicitness. However, on the whole, I don't know that it's a change I want to make now:
- The idea of using
undefined(absence of value) as interpreted to inherit from the parent is consistent with how we're treating color customization above (source) - While not really a blocker in itself, doing this incurs some internal technical complexity because we need to normalize the
'inherit'value back toundefinedin order to effect the inherit behavior by ensuring thatdata-wpds-densityisn't assigned (source) because, unlike[data-wpds-density="compact"], etc., we don't have an explicit selector for the inherited behavior (there is no[data-wpds-density="inherit"]). - The suggestion feels like a logical step in part because of how I've gone out of my way to be explicit here, though in retrospect I'm not fully convinced it was necessary to be so explicit (i.e. including
@default undefinedand includingundefinedin the type union fordensity).
There was a problem hiding this comment.
That's fair. Maybe it looks weird to me because all our "physical" components do @default 'default', and this is the first time we're doing props with inheritance.
* Theme: Fix inline plugin handling of modes * Theme: Add density support for surface padding * UI: Update Box story demonstrating density spacing * Storybook: Add Theme control addon for design system stories * Update theme story to demonstrate density nesting * Remove Compact Box story Now configurable through global theme configuration * Revert changes to Storybook build with dev-ready indicator See: https://github.com/WordPress/gutenberg/pull/73215/files#r2527844132 * Update size options to extend to baseline 4px spacing * Use updated name for small padding token * Ensure default CSS included in Storybook story * Add explicit default density option Allow resetting if unset density would otherwise inherit from a non-default density ancestor * Limit Storybook theming options to design system components Avoid interfering with theming shown in ThemeProvider stories * Improve title of IconButton for Theme toolbar item See: WordPress/gutenberg#73215 (comment) Co-Authored-By: Lena Morita <[email protected]> * Use padding tokens for primary, neutral color demonstration --------- Co-authored-by: Lena Morita <[email protected]> Co-authored-by: aduth <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: mirka <[email protected]> Source: WordPress/gutenberg@599b25e
bph
added
the
Design System
What?
Closes #73360
Updates theme package to implement basic support for density.
Why?
As described in #71196, density is intended to be one of the key configurable aspects of theming. Several of the use-cases described for Boxes associated with #72784 are related to context-specific spacing adjustments (see #72336 (comment)).
How?
Testing Instructions
Verify in Storybook:
npm run storybook:devScreenshots or screencast
Screen.Recording.2025-11-13.at.3.26.04.PM.mov