-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Refactor Admin UI / Breadcrumbs to use DS components and design tokens #77012
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 all commits
f2e6c9b
43b0271
eff0633
6abe3dd
dd69c05
2d067f7
07fc320
a50af82
1543b4c
cf6ce07
4115aa8
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 |
|---|---|---|
| @@ -1,25 +1,19 @@ | ||
| @use "@wordpress/base-styles/variables"; | ||
|
|
||
| .admin-ui-breadcrumbs__list { | ||
| // @TODO: use Text component from UI when available. | ||
| font-family: var(--wpds-typography-font-family-heading); | ||
| font-size: var(--wpds-typography-font-size-lg); | ||
| font-weight: var(--wpds-typography-font-weight-medium); | ||
| line-height: var(--wpds-typography-line-height-lg); | ||
| list-style: none; | ||
| padding: 0; | ||
| margin: 0; | ||
| // gap prop don't seem to be working properly. | ||
| gap: 0; | ||
| min-height: 32px; | ||
|
|
||
| li:not(:last-child)::after { | ||
| content: "/"; | ||
| margin: 0 variables.$grid-unit-10; | ||
| // @TODO: Remove truncation styles once `Text` supports truncation natively. | ||
|
jameskoster marked this conversation as resolved.
|
||
| .admin-ui-breadcrumbs__current { | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| } | ||
|
|
||
| h1 { | ||
| font-size: inherit; | ||
| line-height: inherit; | ||
| .admin-ui-breadcrumbs__separator { | ||
|
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. In other libraries I typically see the separator as non-selectable text, either by SVG image or using
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 don't have a strong feeling about this, but it makes sense to maintain the current behavior. |
||
| color: var(--wpds-color-stroke-surface-neutral); | ||
|
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 figure this is okay as the separator is a decorative element and the list structure conveys meaning to assistive tech. I appreciate it's a bit of a gray area though. Thoughts welcome.
Contributor
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. Does this affect hover styles and general click area of the item, though?
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. No because the separator is attached to the
Contributor
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 the recent changes, actually, I realized that the separator doesn't currently get the same typography styles as the individual items (the Maybe we could consider replacing the Something like: { precedingItems.map( ( item, index ) => (
<li key={ index }>
<Text
variant="body-lg"
render={
<Link
tone="neutral"
render={ <RouterLink to={ item.to } /> }
/>
}
>
{ item.label }
</Text>
<Text variant="body-lg" aria-hidden="true" className="admin-ui-breadcrumbs__separator">
/
</Text>
</li>
) ) }Since This would let us remove the .admin-ui-breadcrumbs__separator {
color: var(--wpds-color-stroke-surface-neutral);
margin: 0 var(--wpds-dimension-gap-sm);
}
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. Nice, yes this makes sense! |
||
| margin: 0 var(--wpds-dimension-gap-sm); | ||
| user-select: none; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| */ | ||
| const ALLOWLIST = { | ||
| '@wordpress/ui': { | ||
| allowed: [ 'Badge', 'Stack', 'Text' ], | ||
| allowed: [ 'Badge', 'Link', 'Stack', 'Text' ], | ||
|
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. If this needs to be handled separately I understand :)
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. Correct 😄 We can unleash these after #76783.
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. Blocked by #77044
Contributor
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. Just flagging that
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. My expectation is that we're okay to make this allowed, but that we'd likely want to accompany this (separately, perhaps) with a more thorough reflection of its status like we did in #77044 (e.g. updating
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'm happy to wait on merging this until |
||
| message: | ||
| '`{{ name }}` from `{{ source }}` is not yet recommended for use in a WordPress environment.', | ||
| }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we discussed or landed on consensus around
Linkcomponent from@wordpress/routeusingLinkfrom@wordpress/ui?(Something to address separately)