[Admin UI]: Move to CSS modules and implement logical properties#77088
[Admin UI]: Move to CSS modules and implement logical properties#77088yogeshbhutkar wants to merge 10 commits intoWordPress:trunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Converts @wordpress/admin-ui styling from package-level global SCSS/build-style imports to component-scoped CSS Modules, while also migrating several layout rules to CSS logical properties (LTR/RTL ready). Updates Storybook and downstream packages to stop importing the Admin UI build-style stylesheet.
Changes:
- Refactor Admin UI
PageandBreadcrumbsstyles into.module.scssfiles and update components to consume CSS Modules. - Remove Admin UI build-style stylesheet loading from Storybook and from other packages’ SCSS entrypoints.
- Update
@wordpress/admin-uiTypeScript config and package metadata to support style module imports.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storybook/package-styles/config.js | Stops lazy-loading Admin UI build-style CSS for admin-ui-* stories. |
| storybook/package-styles/admin-ui-ltr.lazy.scss | Removed Admin UI LTR build-style import file. |
| storybook/package-styles/admin-ui-rtl.lazy.scss | Removed Admin UI RTL build-style import file. |
| packages/interface/src/style.scss | Drops @wordpress/admin-ui/build-style usage from Interface styles aggregation. |
| packages/edit-site/src/style.scss | Drops @wordpress/admin-ui/build-style usage from Edit Site styles aggregation. |
| packages/edit-post/src/style.scss | Drops @wordpress/admin-ui/build-style usage from Edit Post styles aggregation. |
| packages/boot/src/style.scss | Drops @wordpress/admin-ui/build-style usage from Boot styles aggregation. |
| packages/admin-ui/tsconfig.json | Adds style-imports typings for CSS Module imports. |
| packages/admin-ui/src/style.scss | Removes previous package-level SCSS entrypoint that aggregated component styles. |
| packages/admin-ui/src/page/style.module.scss | Introduces CSS Module + logical properties for Page layout and header. |
| packages/admin-ui/src/page/index.tsx | Switches Page to CSS Modules (but currently changes content wrapping behavior). |
| packages/admin-ui/src/page/header.tsx | Switches Header to CSS Modules classes. |
| packages/admin-ui/src/breadcrumbs/style.module.scss | Introduces CSS Module + logical properties for breadcrumbs list styling. |
| packages/admin-ui/src/breadcrumbs/index.tsx | Switches Breadcrumbs to CSS Modules class. |
| packages/admin-ui/README.md | Updates setup instructions to remove Admin UI build-style import guidance. |
| packages/admin-ui/package.json | Updates sideEffects to CSS-module-only SCSS. |
Comments suppressed due to low confidence (1)
packages/admin-ui/src/page/index.tsx:67
- When
hasPaddingis false, the component now returnschildrendirectly (no wrapper). This bypasses the.contentcontainer styles (flex-grow/overflow scrolling), and will likely break layout/scrolling compared to the padded variant. Consider always rendering a content wrapper withstyles.content, and conditionally add thehas-paddingclass when needed.
{ hasPadding ? (
<div
className={ clsx(
styles.content,
styles[ 'has-padding' ]
) }
>
{ children }
</div>
) : (
children
) }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <nav aria-label={ __( 'Breadcrumbs' ) }> | ||
| <HStack | ||
| as="ul" | ||
| className="admin-ui-breadcrumbs__list" |
There was a problem hiding this comment.
With component-scoped styles using modules, we are no longer required to follow the BEM pattern, and the classnames can be simplified.
Question: Do we also keep the existing classnames for back-compat?
There was a problem hiding this comment.
This package is new and bundled, so I think we can drop the existing class names. Let's just note it in the changelog.
| /* stylelint-disable-next-line selector-pseudo-class-no-unknown -- :global is a CSS Modules construct, not a standard pseudo-class */ | ||
| :global(.components-button.has-icon) { |
There was a problem hiding this comment.
":global" will be required to target the class correctly. Therefore, we may need to disable the lint rule here or in the config.
There was a problem hiding this comment.
First, with the new dedicated IconButton component, I think it's now feasible to officially bake in the show-icon-labels feature into the IconButton component itself, to be controlled by the theming system (#61763, cc @WordPress/gutenberg-components).
Second, for the scope of this PR, I'm wondering if we can just leave this out. @yogeshbhutkar can you check whether we really need this .show-icon-labels support at the admin-ui level, at least in how we currently use this package in Gutenberg? Ideally it should live at a higher level, closer to how it's done in the edit-post and edit-site packages.
There was a problem hiding this comment.
First, with the new dedicated
IconButtoncomponent, I think it's now feasible to officially bake in the show-icon-labels feature into theIconButtoncomponent itself, to be controlled by the theming system
Are you imagining this being configurable on the ThemeProvider component and passed down to be considered by the IconButton via React context? I could see that making sense, maybe needing a dedicated implementation ticket related to #61763.
There was a problem hiding this comment.
Can you check whether we really need this .show-icon-labels support at the admin-ui level, at least in how we currently use this package in Gutenberg?
I did a brief audit and found that internal consumers of the Admin UI do rely on the show-icon-labels styles. Specifically, I identified the following:
gutenberg/packages/edit-site/src/components/page-patterns/actions.js
Lines 30 to 32 in 0992bff
gutenberg/routes/styles/stage.tsx
Lines 59 to 62 in 0992bff
Here’s an example screenshot illustrating the impact of removing the show-icon-labels styles:
| Before | After |
|---|---|
![]() |
![]() |
Note: The "show button text labels" property was enabled from editor preferences.
As per your suggestion, should we migrate these to a higher level—such as the corresponding component stylesheets?
There was a problem hiding this comment.
Are you imagining this being configurable on the ThemeProvider component and passed down to be considered by the IconButton via React context? I could see that making sense, maybe needing a dedicated implementation ticket related to #61763.
Yes. That won't solve layout issues (like overflow) automatically, but the basic "hide icon and show text label" part could be handled through the theming system.
There was a problem hiding this comment.
As per your suggestion, should we migrate these to a higher level—such as the corresponding component stylesheets?
Yes, admin-ui itself doesn't add the show-icon-labels class so it shouldn't even know about it. routes/styles doesn't add the class either by the way, so I don't think it's applied there (I'm guessing your screenshot is not from ?page=site-editor-v2).
So that leaves us the two specific instances from edit-site, and I'd say it's reasonable for those call sites to handle the show-icon-labels concerns themselves. I don't think it's safe to handle it globally anyway, given the overflow risks.
Could we perhaps execute this show-icon-labels move in a separate PR first?
|
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. |
| <nav aria-label={ __( 'Breadcrumbs' ) }> | ||
| <HStack | ||
| as="ul" | ||
| className="admin-ui-breadcrumbs__list" |
There was a problem hiding this comment.
This package is new and bundled, so I think we can drop the existing class names. Let's just note it in the changelog.
| content: "/"; | ||
| margin: 0 variables.$grid-unit-10; | ||
| margin-block: 0; | ||
| margin-inline: variables.$grid-unit-10; |
There was a problem hiding this comment.
Let's change this to a design token.
There was a problem hiding this comment.
Let's also change these from scss to css, since we're not using Sass features. This will also enable the stylelints for safer RTL support.
Line 58 in b0a4da8
(I'll add **/*.module.scss to that matcher anyway though.)
| /* stylelint-disable-next-line selector-pseudo-class-no-unknown -- :global is a CSS Modules construct, not a standard pseudo-class */ | ||
| :global(.components-button.has-icon) { |
There was a problem hiding this comment.
Are you imagining this being configurable on the ThemeProvider component and passed down to be considered by the IconButton via React context? I could see that making sense, maybe needing a dedicated implementation ticket related to #61763.
Yes. That won't solve layout issues (like overflow) automatically, but the basic "hide icon and show text label" part could be handled through the theming system.
| /* stylelint-disable-next-line selector-pseudo-class-no-unknown -- :global is a CSS Modules construct, not a standard pseudo-class */ | ||
| :global(.components-button.has-icon) { |
There was a problem hiding this comment.
As per your suggestion, should we migrate these to a higher level—such as the corresponding component stylesheets?
Yes, admin-ui itself doesn't add the show-icon-labels class so it shouldn't even know about it. routes/styles doesn't add the class either by the way, so I don't think it's applied there (I'm guessing your screenshot is not from ?page=site-editor-v2).
So that leaves us the two specific instances from edit-site, and I'd say it's reasonable for those call sites to handle the show-icon-labels concerns themselves. I don't think it's safe to handle it globally anyway, given the overflow risks.
Could we perhaps execute this show-icon-labels move in a separate PR first?
|
|
||
| li:not(:last-child)::after { | ||
| content: "/"; | ||
| margin: 0 variables.$grid-unit-10; |
There was a problem hiding this comment.
Once the stylelints for CSS modules are working, you'll notice that lines like this won't get marked as invalid. This is because 2-value shorthand is safe for RTL. We don't have to split these properties into two.


What?
Closes #77032
Convert the
@wordpress/admin-uipackage to use CSS modules and logical properties.Why?
How?
Testing Instructions
npm run storybook:dev)Screenshots
Note: Before and after changes are expected to be the same.
Use of AI Tools
Used: GH Copilot in ask mode to plan the changes and review them locally.