-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigation: Add post format variation to navigation link block #30403
Conversation
$custom_variation_names = array( | ||
'post_tag' => 'tag', | ||
'post_format' => 'format', | ||
); |
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.
This kind of thing makes me think the variations could be called the same thing as the the entity names.
Size Change: +625 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
{ | ||
name: 'format', | ||
icon: formatIcon, | ||
title: __( 'Post Format Link' ), | ||
description: __( 'A link to a post format.' ), | ||
attributes: { type: 'format' }, | ||
}, |
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 can't seem to make this work without the fallback 🤔
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.
Ah, I see what's happening. First, I need to be on trunk for core. Now after that I have two Tag Link variations. 😕
Wait, so it looks like Post Formats have the same item_link
and item_link_description
as tags 🤯
Someone copied and pasted it years ago and nobody ever noticed?
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.
Hmm, ok, so I think there are some defaults in core for tags or categories:
https://github.com/WordPress/wordpress-develop/blob/0df28171ed9c3c31ebc93add82b64df8f690f87c/src/wp-includes/taxonomy.php#L597-L629
Seems unusual.
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 can't seem to make this work without the fallback
Right any WP instances running 5.7 and below will be using the fallback.
We'll want to update hooks.js or similar to only add this value when post-formats are supported by the theme:
gutenberg/packages/block-library/src/navigation-link/hooks.js
Lines 37 to 44 in 836e6db
// Fallback handling may be deleted after supported WP ranges understand the `variations` | |
// property when passed to register_block_type_from_metadata in index.php | |
if ( ! settings.variations ) { | |
return { | |
...settings, | |
variations: fallbackVariations, | |
}; | |
} |
Shows up in TT1 / WP 5.7 | No Search Results |
---|---|
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.
Irrelevant, but I think TT1 does support formats, but the search only shows results when one is added to a published post
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.
disablePostFormats
is an editor setting from the wordpress/editor
package, so this is tricky to use. The navigation editor doesn't use the editor package, and those settings aren't really relevant to it.
Tempted to either remove it, or keep it as is. The impact is quite low either way.
I think I'll remove it for 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 for taking this one @talldan!
I'm giving tentative approval. One thing that needs to be updated before merge is making sure we don't show the format variations when using the variations fallback. (Eg folks using this on WP <=5.7 with a theme that does not support post formats).
Feel free to land this after that behavior is fixed.
<5.7 Fallback, we see the post format variation when we shouldn't | 5.7+ behavior is good |
---|---|
Some behavior screenshots for future 👀 :
2014 | 2014 + post format results |
---|---|
'name' => 'format', | ||
// The item_link and item_link_description for post formats is the | ||
// same as for tags, so need to be overriden. | ||
'title' => __( 'Post Format Link' ), |
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.
We can also update the registration here to add the post labels. I didn't realize we wanted to show this one, so I missed it in the last PR.
And for folks 👀 changes here, the reason why we added new post labels was because if we combine strings from two translated items, it will generate poor translations.
Example from swissspidy:
In German the translation for A link to a %s. changes depending on the post type.
A link to a post -> Ein Link zu einem Beitrag
A link to a page -> Ein Link zu einer Seite
sprintf( __( 'A link to a %s.' ), $entity->labels->singular_name ) doesn't allow for that.
WP 5.8 is planned for July so I think we can safely make it. (Variation changes aren't present in WP 5.7 and below)
It's okay if we miss the date and need a fallback override, but I'd maybe leave a comment on when it can be removed.
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 push a PR to do that.
The thing I was referring to is how the title and description for tag and category are used as fallbacks by core for any taxonomy that doesn't provide certain labels.
On trunk I'm seeing two Tag Link variations:
The second seems to be post format taxonomy being added as variation but using the title and description of the tag taxonomy (as fallback text).
{ | ||
name: 'format', | ||
icon: formatIcon, | ||
title: __( 'Post Format Link' ), | ||
description: __( 'A link to a post format.' ), | ||
attributes: { type: 'format' }, | ||
}, |
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 can't seem to make this work without the fallback
Right any WP instances running 5.7 and below will be using the fallback.
We'll want to update hooks.js or similar to only add this value when post-formats are supported by the theme:
gutenberg/packages/block-library/src/navigation-link/hooks.js
Lines 37 to 44 in 836e6db
// Fallback handling may be deleted after supported WP ranges understand the `variations` | |
// property when passed to register_block_type_from_metadata in index.php | |
if ( ! settings.variations ) { | |
return { | |
...settings, | |
variations: fallbackVariations, | |
}; | |
} |
Shows up in TT1 / WP 5.7 | No Search Results |
---|---|
'title' => __( 'Post Format Link' ), | ||
'description' => __( 'A link to a post format' ), | ||
'attributes' => array( | ||
'type' => 'format', |
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 don't have strong feelings on this one, but since post_format
is a less used feature, I think it'd be okay to prefer the slug name of post_format
for storing the type value.
This field isn't exposed to the user unless they're editing the code view directly.
For context, I had to keep the tag/post_tag
exception in the past due to the value being saved in earlier navigation link versions.
@@ -117,6 +117,8 @@ function getSuggestionsQuery( type, kind ) { | |||
return { type: 'term', subtype: 'category' }; | |||
case 'tag': | |||
return { type: 'term', subtype: 'post_tag' }; | |||
case 'format': | |||
return { type: 'post-format' }; |
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.
For folks wondering about the kebab case:
if ( ! disablePostFormats && ( ! type || type === 'post-format' ) ) { |
FYI on behavior, it also looks like we removed the labels in search results if all results are the same:
#24839
So this is expected behavior:
Custom Link Search | No labels with all post formats |
---|---|
Thanks for the review @gwwar! |
Description
Assists with #29793.
Adds a Post Format variation to that navigation link block.
#22600 added the ability to search for a post format, but it appears this was never turned into a block variation, so post formats can only be created as custom links.
This new variation needs an icon. At the moment it just shows the custom post type icon. But I think it's fine to merge like this and add the icon later.
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.js
files for terms that need renaming or removal).