-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Breadcrumbs: Add filter for preferred taxonomy and term per post type #73283
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
Breadcrumbs: Add filter for preferred taxonomy and term per post type #73283
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. |
mcsf
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.
Looks good overall, and I'd be up for merging this, but I still raised a question about hooks vs. any alternatives.
That said, hooking by post type seems totally reasonable.
|
|
||
| // Find the first taxonomy that has terms assigned to this post. | ||
| /** | ||
| * Filters the preferred taxonomy and term for breadcrumbs on a per-post-type basis. |
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.
High-level question, and also a non-leading one: how does this hook-based solution weigh up against one relying on user-editable block attributes? I'm not implying that they are mutually exclusive, but this seems like useful justification for this PR.
Is this hook something requested or validated by precedents in x3p0-breadcrumbs?
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.
Some plugins (Woo breadcrumbs) have filters and others a UI(or maybe both). As you said they are not mutually exclusive, but I think plugin authors would mostly use that filter during registration of post types/taxonomies. So I see value in having a filter.
Having said that, we can add the UI later on too.
Do you think we should only have a UI for these?
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 think just the hook is fine for now. These scenarios are all a bit abstract in my head, hence all the hesitation in my questions. :)
| * Allows developers to specify which taxonomy and term should be used in breadcrumbs | ||
| * when a post has multiple taxonomies or terms assigned. | ||
| * | ||
| * The preferred taxonomy must have terms assigned to the post, otherwise falls back to the first available taxonomy. | ||
| * | ||
| * The preferred term must exist and be assigned to the post. If the post has only one term, it's used regardless of preference. | ||
| * If the preferred term is not found, falls back to the first term. |
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.
Really off-topic, but I wish we'd introduce a style guideline on comment length. (Me, I'm old school and still format mine at 79–80 chars depending on the place, but anything between 72 and 90-ish would make me happy.)
| * } | ||
| * @param string $post_type The post type slug. | ||
| */ | ||
| $preferences = apply_filters( 'block_core_breadcrumbs_post_type_preferences', array(), $post_type ); |
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.
Two thoughts on the name:
- The name is more generic than the function calling it (
block_core_breadcrumbs_get_terms_breadcrumbs). Is that intentional, so that the same hook could be used in other Breadcrumbs functions (thereby allowing devs to hook once to affect multiple scenarios)? If not, I'd say the name should better reflect the caller. - On the back of my mind: the term "preference" is a little loaded, but I don't have any better alternatives. For instance, "default" isn't the same thing at all: it's something to use if there are no current values.
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.
The name is more generic than the function calling it (block_core_breadcrumbs_get_terms_breadcrumbs). Is that intentional,
Yes. I was trying to think what kind of filters we would want in general for this block and if we should create one (for many cases) or more specialized filters. I concluded that we might want more specialized filters, but grouped by functionality.
In this case, the filter is about post_type preferences and now is used for taxonomy/term, it can immediately be used for displaying or not the archive label though, and possibly other settings in the future (related PR).
On the back of my mind: the term "preference" is a little loaded,
I guess we could go with settings too. I'll update.
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.
it can immediately be used for displaying or not the archive label though, and possibly other settings in the future (related PR).
Do you mean an option to tell Breadcrumbs not to display the archive label even though the post type has has_archive set? What's the use case? Or did I misunderstand?
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.
Yes, that's what I meant. TBH that was my personal thinking and I realize that it was mostly related with what I'm expecting usually in sites with products. For products cases I mostly use breadcrumb navigation to visit parent/current categories of the product.
But you're right, that this might not be needed to be a setting. At least for start.. We could consider it if it's requested.
So regarding the naming we could go with settings for now. We have some time for testing before stabilizing and we can revisit and change the name.
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.
By the way, I really appreciate your questions, because there are not many people involved in these breadcrumbs conversations and it becomes clear to me that while I'm doing my best to think of all the use cases and needs, I'm being subjective without even realizing it some times.
329f749 to
54b64bc
Compare
mcsf
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.
🚢 ❗
54b64bc to
089590e
Compare
What?
Part of: #72649
Add a PHP filter for users to set their preferred main taxonomy and term per post type. This is needed to have control over the breadcrumbs trail in case a post has multiple taxonomies and/or terms assigned.
This PR Introduces the
block_core_breadcrumbs_post_type_settingsPHP filter to allow developers to specify which taxonomy and term should be used in breadcrumbs when a post has multiple taxonomies or terms assigned.Parameters: $post_type (string): The post type slug
Returns: (array) Settings array with the following keys:
taxonomy(string)term(string)Fallback behavior:
Example
Testing Instructions
You can create your own taxonomies/terms to test, but specifically for the above snippet you'll need:
For the
postpost typeCreate a couple of categories and assign them to a post. Replace
$settings['term'] = 'news';with one of your created categoriesslug. See the front end.For the
productpost type from Wooproduct_tag.Screenshots or screencast
Below I'm sharing some screenshots in a site that I've added x3p0-breadcrumbs plugin and the core block in the main header.
Above is x3p0-breadcrumbs and below is the core block.
Preferred taxonomy
product_tagfor Woo productsPreferred term
phonesfor posts