admin-ui / Breadcrumbs: stricter items[].to prop types#76493
Merged
Conversation
|
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. |
Contributor
Author
|
cc @CGastrell |
ciampo
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 7.66 MB ℹ️ View Unchanged
|
ciampo
changed the title
items[].to prop
ciampo
changed the title
items[].to propitems[].to prop types
ciampo
force-pushed
the
feat/admin-ui-breadcrumb-types
branch
from
63a5d94 to
ff216bd
Compare
Contributor
|
Worked well on my end, trying to add an item without First item without Adding these lines to the Breadcrumbs component (green margin) seem well scoped: And produces this: So, even with a failing item, the UI doesn't break. Unsure this is something we want to consider, I just think of prioritizing a UI that doesn't break. I'll leave it for your consideration. Other than that, I have no authority here to approve the PR, but it works well. |
ciampo
force-pushed
the
feat/admin-ui-breadcrumb-types
branch
from
ff216bd to
709c2d2
Compare
Contributor
Author
|
The TS error should already be a strong indication (and should ideally prevent the build from completing).
Most of those lines should be redundant, since the Apart from that, fixing styles would not remove the main semantics issue (two or more |
Contributor
Author
|
Separately, I believe
Not sure who owns this, but cc @WordPress/gutenberg-components @WordPress/gutenberg-design |
|
Flaky tests detected in 2ce11c6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23544570041
|
ciampo
force-pushed
the
feat/admin-ui-breadcrumb-types
branch
from
b4cf231 to
0a34378
Compare
Contributor
|
Would it be better to render the |
Contributor
Author
Yes: IMO, breadcrumbs should be about navigation (and should not provide a page heading). I think we have to sit down and gather all requirements for breadcrumbs and page headers in general, so that we can come up with a solution that is semantic, accessible, comprehensive, and flexible. We could also think of exposing a |
Contributor
|
I think the breadcrumb options can start quite simple. We basically just need a way to visually hide the current page, and to specify whether it's a link or not. I agree it should be about navigation and nothing else. The page title should be separate, even if it's composed in a way that looks like it's part of the breadcrumb in page headers: 
|
Contributor
Author
I'm afraid that asking consumers to implement this layout on their end may lead to many inconsistencies. Alternatively, we could:
What do you think? |
Contributor
IMHO, the page title and breadcrumbs are a matter of semantics and composing. As @ciampo mentions, breadcrumbs are a navigational artifact that should be semantically and structurally correct. In terms of page readers, the title is the main descriptor of where the visitor "is". While breadcrumbs can describe a "path" to the current place, it might not be as useful. So, a couple thoughts:
|
Contributor
I wasn't suggesting that we should ask consumers to implement it, the idea was that it would be built into the page header component somehow. It sounds like we're suggesting the same thing, just a slightly different implementation method. I was saying "visually hide the current page in the breadcrumb, show the page title", you're saying "visually hide the page title, show the full breadcrumb trail including the current page". Visually it amounts to the same thing, so I'm happy to defer to y'all on which approach is best! |
Contributor
Author
|
I'm quite convinced about having Not sure about having Regarding who does the "orchestration" between all the smaller parts: whether it's |
jameskoster
reviewed
Mar 18, 2026
| { lastItem.to ? ( | ||
| <Link to={ lastItem.to }>{ lastItem.label }</Link> | ||
| ) : ( | ||
| <Heading level={ 1 } truncate> |
Contributor
There was a problem hiding this comment.
Should we use Text here?
Contributor
Author
There was a problem hiding this comment.
I'd prefer to make the change in a separate PR, to keep scope tight here — would that be ok?
ciampo
force-pushed
the
feat/admin-ui-breadcrumb-types
branch
from
0a34378 to
8d2f8ee
Compare
Contributor
Author
|
@jameskoster @CGastrell @WordPress/gutenberg-components Do you think this PR is valuable to merge? If so, can it have a final review round? |
Contributor
|
Yes, I think if the restriction makes sure the components behaves and renders consistently, it's not only worth it, it's a good way of helping folks realize of how it should be used. |
Contributor
The comment aimed reinforce the other one, pointing out the fact that const items = [ { to: '/somepath' } ];
const precedingItems = items.slice( 0, -1 );
// At this point, precedingItems is the same as items: [ { to: '/somepath' } ]
const lastItem = items[ items.length - 1 ];
// Again, lastItem is the same as items and precedingItems
// ... later on, we iterate precedingItems and render the element
<Link to={ item.to }>{ item.label }</Link>
// And right next to it, we render the lastItem with either Link or Heading
<Link to={ lastItem.to }>{ lastItem.label }</Link>I'm not sure how we want to sort that out, but a single item breadcrumbs array might get into a weird situation. We could also leave that to the consumer's consideration. |
Contributor
|
@ciampo I don't think there's much for me to add from a design perspective, but let me know if there's somethign specific you'd like feedback on. |
Contributor
Author
Performing |
Contributor
I stand corrected, sorry I misread that! |
CGastrell
approved these changes
Mar 21, 2026
Contributor
CGastrell
left a comment
CGastrell
left a comment
There was a problem hiding this comment.
LGTM! We'll iterate any further needs! Thanks!
ciampo
force-pushed
the
feat/admin-ui-breadcrumb-types
branch
from
bcf81cf to
16bcf6c
Compare
adamsilverstein
pushed a commit
that referenced
this pull request
Mar 26, 2026
* admin-ui/Breadcrumbs: enforce "to" prop on all but last item * Add JSDoc * Add type tests * build docs * Allow the rendering of an h1 only for the last item * Update docs * Format * fix eslint error * CHANGELOG * Auto-format * Remove unnecessary `key` attribute * Use runtime check (throws error in dev mode) instead of stricter types * Better error message * format * remove unused import --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: CGastrell <cgastrell@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>
adamsilverstein
pushed a commit
that referenced
this pull request
Mar 31, 2026
* admin-ui/Breadcrumbs: enforce "to" prop on all but last item * Add JSDoc * Add type tests * build docs * Allow the rendering of an h1 only for the last item * Update docs * Format * fix eslint error * CHANGELOG * Auto-format * Remove unnecessary `key` attribute * Use runtime check (throws error in dev mode) instead of stricter types * Better error message * format * remove unused import --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: CGastrell <cgastrell@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>
adamsilverstein
pushed a commit
that referenced
this pull request
Apr 7, 2026
* admin-ui/Breadcrumbs: enforce "to" prop on all but last item * Add JSDoc * Add type tests * build docs * Allow the rendering of an h1 only for the last item * Update docs * Format * fix eslint error * CHANGELOG * Auto-format * Remove unnecessary `key` attribute * Use runtime check (throws error in dev mode) instead of stricter types * Better error message * format * remove unused import --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: CGastrell <cgastrell@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Related to #76467
Tighten types in admin-ui's
Breadcrumbscomponent to clarify expectations around all items (apart from the last one) to have to specify atopropWhy?
Current types don't specify that requirement, and it's possible to render multiple
<h1>breadcrumb items — which is wrong semantically and breaks the layoutHow?
h1when it has notopropTesting Instructions
Breadcrumbswithout settingtoto one of the (non-last) items — a TS error should be flagged