-
Notifications
You must be signed in to change notification settings - Fork 4.8k
[Admin UI]: Move to CSS modules and implement logical properties #77088
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
base: trunk
Are you sure you want to change the base?
Changes from 9 commits
63843a5
8ff22d4
5621cf8
915452e
0225799
6d3fbf2
804cc0e
1acd2af
5bb85d5
0992bff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { | |
| * Internal dependencies | ||
| */ | ||
| import type { BreadcrumbsProps } from './types'; | ||
| import styles from './style.module.scss'; | ||
|
|
||
| /** | ||
| * Renders a breadcrumb navigation trail. | ||
|
|
@@ -56,7 +57,7 @@ export const Breadcrumbs = ( { items }: BreadcrumbsProps ) => { | |
| <nav aria-label={ __( 'Breadcrumbs' ) }> | ||
| <HStack | ||
| as="ul" | ||
| className="admin-ui-breadcrumbs__list" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This package is new and bundled, so I think we can drop the existing class names. Let's just note it in the changelog. |
||
| className={ styles.list } | ||
| spacing={ 0 } | ||
| justify="flex-start" | ||
| alignment="center" | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Matcher broadened in #77140 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| @use "@wordpress/base-styles/variables"; | ||
|
|
||
| .admin-ui-breadcrumbs__list { | ||
| .list { | ||
| // @TODO: use Text component from UI when available. | ||
| font-family: var(--wpds-font-family-heading); | ||
| font-size: var(--wpds-font-size-lg); | ||
|
|
@@ -15,7 +15,8 @@ | |
|
|
||
| li:not(:last-child)::after { | ||
| content: "/"; | ||
| margin: 0 variables.$grid-unit-10; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| margin-block: 0; | ||
| margin-inline: variables.$grid-unit-10; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's change this to a design token. |
||
| } | ||
|
|
||
| h1 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||
| .admin-ui-page { | ||||||||||||||||||||||||||
| .page { | ||||||||||||||||||||||||||
| display: flex; | ||||||||||||||||||||||||||
| height: 100%; | ||||||||||||||||||||||||||
| // @TODO: Revisit page background color. Consider using | ||||||||||||||||||||||||||
|
|
@@ -13,16 +13,17 @@ | |||||||||||||||||||||||||
| text-wrap: pretty; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| .admin-ui-page__header { | ||||||||||||||||||||||||||
| padding: var(--wpds-dimension-padding-md) var(--wpds-dimension-padding-2xl); | ||||||||||||||||||||||||||
| border-bottom: var(--wpds-border-width-xs) solid var(--wpds-color-stroke-surface-neutral-weak); | ||||||||||||||||||||||||||
| .header { | ||||||||||||||||||||||||||
| padding-block: var(--wpds-dimension-padding-md); | ||||||||||||||||||||||||||
| padding-inline: var(--wpds-dimension-padding-2xl); | ||||||||||||||||||||||||||
| border-block-end: var(--wpds-border-width-xs) solid var(--wpds-color-stroke-surface-neutral-weak); | ||||||||||||||||||||||||||
| background: var(--wpds-color-bg-surface-neutral-strong); | ||||||||||||||||||||||||||
| position: sticky; | ||||||||||||||||||||||||||
| top: 0; | ||||||||||||||||||||||||||
| inset-block-start: 0; | ||||||||||||||||||||||||||
| z-index: 1; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| .admin-ui-page__header-title { | ||||||||||||||||||||||||||
| .header-title { | ||||||||||||||||||||||||||
| // @TODO: use Text component from UI when available. | ||||||||||||||||||||||||||
| font-family: var(--wpds-font-family-heading); | ||||||||||||||||||||||||||
| font-size: var(--wpds-font-size-lg); | ||||||||||||||||||||||||||
|
|
@@ -34,35 +35,39 @@ | |||||||||||||||||||||||||
| white-space: nowrap; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| .admin-ui-page__sidebar-toggle-slot:empty { | ||||||||||||||||||||||||||
| .sidebar-toggle-slot:empty { | ||||||||||||||||||||||||||
| display: none; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| .admin-ui-page__header-subtitle { | ||||||||||||||||||||||||||
| .header-subtitle { | ||||||||||||||||||||||||||
| padding-block-end: var(--wpds-dimension-padding-xs); | ||||||||||||||||||||||||||
| color: var(--wpds-color-fg-content-neutral-weak); | ||||||||||||||||||||||||||
| font-size: var(--wpds-font-size-md); | ||||||||||||||||||||||||||
| line-height: var(--wpds-font-line-height-md); | ||||||||||||||||||||||||||
| margin: 0; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| .admin-ui-page__content { | ||||||||||||||||||||||||||
| .content { | ||||||||||||||||||||||||||
| flex-grow: 1; | ||||||||||||||||||||||||||
| overflow: auto; | ||||||||||||||||||||||||||
| display: flex; | ||||||||||||||||||||||||||
| flex-direction: column; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| &.has-padding { | ||||||||||||||||||||||||||
| padding: var(--wpds-dimension-padding-lg) var(--wpds-dimension-padding-2xl); | ||||||||||||||||||||||||||
| padding-block: var(--wpds-dimension-padding-lg); | ||||||||||||||||||||||||||
| padding-inline: var(--wpds-dimension-padding-2xl); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Show button text labels when preference is enabled. | ||||||||||||||||||||||||||
| .show-icon-labels { | ||||||||||||||||||||||||||
| .admin-ui-page__header-actions { | ||||||||||||||||||||||||||
| .components-button.has-icon { | ||||||||||||||||||||||||||
| /* stylelint-disable-next-line selector-pseudo-class-no-unknown -- :global is a CSS Modules construct, not a standard pseudo-class */ | ||||||||||||||||||||||||||
| :global(.show-icon-labels) { | ||||||||||||||||||||||||||
| .header-actions { | ||||||||||||||||||||||||||
| /* 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) { | ||||||||||||||||||||||||||
|
Comment on lines
+66
to
+67
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ":global" will be required to target the class correctly. Therefore, we may need to disable the lint rule here or in the config.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First, with the new dedicated 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I did a brief audit and found that internal consumers of the Admin UI do rely on the gutenberg/packages/edit-site/src/components/page-patterns/actions.js Lines 30 to 32 in 0992bff
gutenberg/packages/edit-site/src/components/sidebar-global-styles/index.js Lines 35 to 37 in 0992bff
gutenberg/routes/styles/stage.tsx Lines 59 to 62 in 0992bff
Here’s an example screenshot illustrating the impact of removing the
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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, admin-ui itself doesn't add the 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 Could we perhaps execute this |
||||||||||||||||||||||||||
| width: auto; | ||||||||||||||||||||||||||
| padding: 0 var(--wpds-dimension-padding-xs); | ||||||||||||||||||||||||||
| padding-block: 0; | ||||||||||||||||||||||||||
| padding-inline: var(--wpds-dimension-padding-xs); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Hide the button icons when labels are set to display... | ||||||||||||||||||||||||||
| svg { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
This file was deleted.
This file was deleted.
This file was deleted.


Uh oh!
There was an error while loading. Please reload this page.