-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Breadcrumbs: Add home page handling and show last item attribute
#72839
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
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. |
|
Size Change: +158 B (+0.01%) Total Size: 2.45 MB
ℹ️ View Unchanged
|
|
Seems valid enough. "Display breadcrumbs on the site front page and blog posts index" help text reads sligthly roundabout. If we're referring to the template types on which it will show, we should probably refer to those directly. Something like (but please correct me if I get this wrong):
|
@jasmussen after thinking about it more, I'm not sure we should mention templates because there can be cases a user assigns a specific page for his home page and Maybe the best path forward is the reduction of the message like the label does I think it's fine to refine text in follow ups too, but for me it's more important for this PR to explore or validate the initial design and/or placement of these settings. Is it fine to start with them under advanced like this PR or we should explore something different? |
|
I like the simple "Show on home page", with no help text. 👍 👍 |
a5a624d to
7c7a486
Compare
|
Flaky tests detected in 6a44a60. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19031229602
|
e6722c4 to
146e62d
Compare
146e62d to
6a44a60
Compare
tyxla
left a comment
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.
Looking good from my perspective 👍 Thanks!
| if ( $is_home_or_front_page ) { | ||
| $breadcrumb_items[] = sprintf( | ||
| '<span aria-current="page">%s</span>', | ||
| esc_html__( 'Home' ) |
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.
I guess we might want to land this after #72873 to reflect the escaping changes
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.
I'll have conflicts either way, so I'll land and rebase the other now.
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.
Thanks, let me know if #72873 needs another 👁️
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.
I rebased and it seems fine. I'll merge tomorrow in case you do want to take another look at that PR. Thanks!
What?
Part of: #72498 and #72649
This PR does a couple of things:
is_home() && is_paged()it also shows the page trail. This setting is only useful when the user would want to have a single breadcrumbs block in the site header and is similar toPrefer taxonomy termssetting. That's the reason I've added it underadvanced. We could leave it there or we could consider a different design for showing/editing these two attributes. --cc @jasmussenTesting Instructions
show last itemattributeonein reading settings.Screenshots or screencast