-
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
Categories List block: Add dropdown for taxonomies #65272
Changes from all commits
8adf0ea
0ba38ab
e9e469a
c23f489
1ed944e
3884104
3a059a8
c612513
a67ae65
a903837
d960af4
f44da51
06e8b27
b4e20f1
238d180
cba5a9d
a14c679
47bbbfb
6934817
25649b2
95a6cf3
061f501
923da7a
879ae09
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 |
---|---|---|
|
@@ -21,11 +21,14 @@ function render_block_core_categories( $attributes, $content, $block ) { | |
static $block_id = 0; | ||
++$block_id; | ||
|
||
$taxonomy = get_taxonomy( $attributes['taxonomy'] ); | ||
|
||
$args = array( | ||
'echo' => false, | ||
'hierarchical' => ! empty( $attributes['showHierarchy'] ), | ||
'orderby' => 'name', | ||
'show_count' => ! empty( $attributes['showPostCounts'] ), | ||
'taxonomy' => $attributes['taxonomy'], | ||
'title_li' => '', | ||
'hide_empty' => empty( $attributes['showEmpty'] ), | ||
); | ||
|
@@ -36,13 +39,20 @@ function render_block_core_categories( $attributes, $content, $block ) { | |
if ( ! empty( $attributes['displayAsDropdown'] ) ) { | ||
$id = 'wp-block-categories-' . $block_id; | ||
$args['id'] = $id; | ||
$args['show_option_none'] = __( 'Select Category' ); | ||
$show_label = empty( $attributes['showLabel'] ) ? ' screen-reader-text' : ''; | ||
$default_label = __( 'Categories' ); | ||
$label_text = ! empty( $attributes['label'] ) ? $attributes['label'] : $default_label; | ||
$wrapper_markup = '<div %1$s><label class="wp-block-categories__label' . $show_label . '" for="' . esc_attr( $id ) . '">' . $label_text . '</label>%2$s</div>'; | ||
$items_markup = wp_dropdown_categories( $args ); | ||
$type = 'dropdown'; | ||
$args['name'] = $taxonomy->query_var; | ||
$args['value_field'] = 'slug'; | ||
$args['show_option_none'] = sprintf( | ||
/* translators: %s: taxonomy's singular name */ | ||
__( 'Select %s' ), | ||
$taxonomy->labels->singular_name | ||
); | ||
|
||
$show_label = empty( $attributes['showLabel'] ) ? ' screen-reader-text' : ''; | ||
$default_label = $taxonomy->label; | ||
$label_text = ! empty( $attributes['label'] ) ? $attributes['label'] : $default_label; | ||
$wrapper_markup = '<div %1$s><label class="wp-block-categories__label' . $show_label . '" for="' . esc_attr( $id ) . '">' . $label_text . '</label>%2$s</div>'; | ||
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. See comment from @peterwilsoncc WordPress/wordpress-develop#7226 (comment)
It was a preexisting issue, though. |
||
$items_markup = wp_dropdown_categories( $args ); | ||
$type = 'dropdown'; | ||
|
||
if ( ! is_admin() ) { | ||
// Inject the dropdown script immediately after the select dropdown. | ||
|
@@ -54,6 +64,8 @@ function render_block_core_categories( $attributes, $content, $block ) { | |
); | ||
} | ||
} else { | ||
$args['show_option_none'] = $taxonomy->labels->no_terms; | ||
|
||
$wrapper_markup = '<ul %1$s>%2$s</ul>'; | ||
$items_markup = wp_list_categories( $args ); | ||
$type = 'list'; | ||
|
@@ -92,8 +104,8 @@ function build_dropdown_script_block_core_categories( $dropdown_id ) { | |
( function() { | ||
var dropdown = document.getElementById( '<?php echo esc_js( $dropdown_id ); ?>' ); | ||
function onCatChange() { | ||
if ( dropdown.options[ dropdown.selectedIndex ].value > 0 ) { | ||
location.href = "<?php echo esc_url( home_url() ); ?>/?cat=" + dropdown.options[ dropdown.selectedIndex ].value; | ||
if ( dropdown.options[ dropdown.selectedIndex ].value !== -1 ) { | ||
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. There is an opportunity to refactor this block to use Interactivity API. @DAreRodz and @michalczaplinski, was it on the radar? |
||
location.href = "<?php echo esc_url( home_url() ); ?>/?" + dropdown.name + '=' + dropdown.options[ dropdown.selectedIndex ].value; | ||
} | ||
} | ||
dropdown.onchange = onCatChange; | ||
|
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.
Given that we are changing the name of the block here I would recommend we at least add
Categories
to thekeywords
array so it still appears in search.My personal recommendation however would be that for backwards compatibility we register one singular block variation for the category taxonomy. This way we are not renaming an existing block. Users still get the same result. But they now have an additional control to change the block to query other taxonomies.
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.
Good point 👍
Interesting. I gave this a quick spin (see below diff). I'm assuming you mean with
scope
set to excludetransform
, so that there won't be the dropdown to transform variations (as there is in #64805) as it'd be otherwise confusing to have two dropdowns to control which variation to display.This posed the following problem: For back-compat, we need to keep
category
as the default value for thetaxonomy
attribute, so that any existing occurrences of<!-- wp:categories /-->
will continue to be rendered as the Categories List variation.But that raises the question of the default value for that attribute when inserting a new instance of the generic Terms List variation. It cannot be omitted, as that would translate to
category
(per the above), causing the block to be promptly rendered as the Categories List variation.So we need a different default value for the Terms List variation. I guess Tags -- the only other built-in and public taxonomy -- is the logical contender here, so this is what I chose.
Furthermore, there's the question of the dropdown. IMO, it needs to be hidden altogether in case of the Categories List variation, as it would otherwise allow to transform it to the Terms List variation. In the same vein, the "Categories" option needs to be removed from the dropdown in case of the Terms List variation, to prevent transforming into the Categories List variation.
But that means that the Terms List block is now solely for Tags and custom taxonomies, whereas Categories get their own variation (which looks like a separate block, for all intents and purposed). This feels a bit arbitrary IMO; I'm not sure it's that much better in terms of UX than not having the separate Categories List block variation.
Diff
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 at the diff I would not remove the
category
option from the dropdown of available taxonomies. If a user happens to insert the "Term List" block and then selects the "Category" taxonomy in the drop-down I'd be fine with that changing the name of the block to the correct variation.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.
At the same time I would be fine with users switching from the Category List to another 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.
TBH, that would feel inconsistent to me in terms of UX; I think it would be somewhat surprising to the user if selecting "Categories" as the taxonomy from that dropdown gets special treatment, while all other taxonomies don't 😕
If it's not too much of a dealbreaker for you, I'd rather consider the idea of a dedicated variation for the Categories List block separately -- i.e. in a follow-up PR, where it can also get the attention by designers that it deserves, without conflating it with the principal subject of this PR of adding the
taxonomy
attribute and dropdown in the first place 😊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.
Thank you!
I've added
"keywords": [ "categories" ]
in 95a6cf3.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 looks like block variations were discussed extensively in:
One drawback is that users won't see Categories in the inserters when typing
categories
term. The icon remains the same, so maybe that isn't that much of an issue, as with thekeywords
, it will remain discoverable. Although, we should follow the feedback closely during beta and RC phase of the release.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.
That's a fair point and one that I hadn't considered; I've shared some more thoughts below.
I'll add one more note for posterity: With the suggested change we've been discussing in this thread -- i.e. adding two block variations, one for Categories specifically, and a "generic" one for all other cases -- there can also be some confusion in the inserter: I've called the "generic" variation "Terms List", just like the block itself; but since both the block itself and all its variations are shown in the inserter, there would be two "Terms List" blocks:
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've filed a draft PR for this regardless, in case we'd like to revisit this: #65434
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.
Turns out I was wrong about this one: #65434 (comment) 😅