[WIP] Add ability to target Nav block current menu item via Theme JSON #54460
Conversation
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/class-wp-theme-json-gutenberg.php ❔ phpunit/class-wp-theme-json-test.php |
| "test:unit:php:base": "wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-wordpress vendor/bin/phpunit -c phpunit.xml.dist --verbose", | ||
| "test:unit:php:base": "wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-wordpress vendor/bin/phpunit -c phpunit.xml.dist --verbose --filter WP_Theme_JSON_Gutenberg_Test", |
There was a problem hiding this comment.
@anton-vlasenko We weren't able to run the PHP unit tests with the --filter option. We tried:
npm run test:unit:php -- --filter WP_Theme_JSON_Gutenberg_Test
...but it says "Invalid option: filter".
Any ideas why this might be happening?
There was a problem hiding this comment.
This is passing the --filter to npm-run-all instead of phpunit.
I would expect this to work:
npm run wp-env start
npm run test:unit:php:base -- --filter WP_Theme_JSON_Gutenberg_Test
There was a problem hiding this comment.
This is correct @ajlende
There was a problem hiding this comment.
Maybe it's the base that allows this to work?
But I should be able to call npm run test:unit:php without the :base like I could previously.
| @@ -785,6 +789,9 @@ protected static function sanitize( $input, $valid_block_names, $valid_element_n | |||
| $schema_styles_blocks[ $block ] = $styles_non_top_level; | |||
| $schema_styles_blocks[ $block ]['elements'] = $schema_styles_elements; | |||
| $schema_styles_blocks[ $block ]['variations'] = $schema_styles_variations; | |||
| $schema_styles_blocks[ $block ]['.current-item'] = $styles_non_top_level; | |||
There was a problem hiding this comment.
The value .current-item should come from a whitelist of allowed "state selectors". The terminology we use here will be critical.
I think the selectors should be open to any block type but I'm open to being told that's wrong.
| @@ -2283,8 +2297,17 @@ private static function get_block_nodes( $theme_json, $selectors = array() ) { | |||
| 'selectors' => $feature_selectors, | |||
There was a problem hiding this comment.
I'm wondering whether these 'feature_selectors' can help us.
Need more info on how/where they are output.
| @@ -2283,8 +2297,17 @@ private static function get_block_nodes( $theme_json, $selectors = array() ) { | |||
| 'selectors' => $feature_selectors, | |||
| 'duotone' => $duotone_selector, | |||
| 'variations' => $variation_selectors, | |||
| // 'state_selectors' => $selectors[ $name ]['.current-item'], | |||
There was a problem hiding this comment.
It may be that we need to create another set of sub-selectors under a new key. This would house any further "state" selectors like current-item.
|
Size Change: +38 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
|
||
| $nodes[] = array( | ||
| 'name' => 'core/navigation', | ||
| 'path' => array( 'styles', 'blocks', 'core/navigation', '.current-item' ), |
There was a problem hiding this comment.
This is a hack.
We can either:
- have the code that processes the block
$nodeunderstand how to handle styles for.current-item. - add a new loop in this location which processes it (a bit like how pseudo elements are handled below).
I prefer the former approach as it's more declarative.
|
Most recent change iterates towards using the Selectors API. We will have to update Theme JSON in order that it knows how to get the selector for |
| @@ -134,6 +134,9 @@ | |||
| }, | |||
| "interactivity": true | |||
| }, | |||
| "selectors": { | |||
| "@currentItem": ".wp-block-navigation .current-item" | |||
There was a problem hiding this comment.
| "@currentItem": ".wp-block-navigation .current-item" | |
| "@currentItem": ".wp-block-navigation .current-menu-item" |
For backwards compat reasons we'll want the selector to match what the block does currently.
phpunit/class-wp-theme-json-test.php
Outdated
| $this->assertEquals( $expected, $theme_json->get_stylesheet() ); | ||
| $this->assertEquals( $expected, $theme_json->get_stylesheet( array( 'styles' ) ) ); |
There was a problem hiding this comment.
Let's update these so that they strip out the base styles before the assertion is run so that when the test fails we have a smaller diff.
|
Next step here is to be able to handle nesting and declaration (including |
|
@aaronrobertshaw I wonder if we could solicit your advice here? We are attempting to allow Theme authors to be able to target a "current item" state within a block via A good example is the Navigation block where you want to be able to style the navigation item that matches the current page you are viewing. We are currently piggybacking on the Selectors API to achieve this, mapping Our problem is that we need to be able to define states for Our main question is whether the Selectors API a good choice to use for this feature or should be roll our own custom logic? |
| @@ -3597,6 +3604,14 @@ protected function get_feature_declarations_for_node( $metadata, &$node ) { | |||
| // to leverage existing `compute_style_properties` function. | |||
| $feature_node = array( $feature => $node[ $feature ] ); | |||
|
|
|||
| // If the feature is a state selector (e.g. `@currentItem`) | |||
There was a problem hiding this comment.
I wonder if we should consider current item a state? It's not a state really (like say hover is), it's just a special selector.
There was a problem hiding this comment.
Yeh we need to get the nomenclature for this right now. If we put "state" in the code the feature will be known as "state".
Still...I can't think of a better term.
I asked an AI assistance and it came up with
"It's currently in use."
"It's presently active."
"It's in operation."
"It's functioning now."
"It's currently live."
"It's currently enabled."
"It's in effect."
"It's the current choice."
"It's presently selected."
"It's the current option."
```
There was a problem hiding this comment.
what is the name of core/navigation node in theme json? that is the same as core/navigation > currentItem
There was a problem hiding this comment.
what is the name of core/navigation node in theme json? that is the same as core/navigation > currentItem
Sorry I don't understand what you are asking 🙏
There was a problem hiding this comment.
How do we refer to nodes like core/navigation (a block name) in theme json, what kind of node is it? (style, properties?). Becasue the @current node is an override of the parent - not a state.
There was a problem hiding this comment.
Thanks for the ping 👍
I might need to take some time to get a proper grasp of what you're proposing here and how the Selectors API might be leveraged. I'm happy to share some initial thoughts in the meantime though, if that helps.
We are attempting to allow Theme authors to be able to target a "current item" state within a block via theme.json.
After taking a quick look at this PR and surrounding conversation, it appears as though these "states" like "current item" are more akin to a block style variation than a feature such as typography, colors etc. Which is what the Selectors API was aimed at.
In other words, a currentItem state needs to support all the styling options that a full block type does. Including features like typography, elements, element pseudo selectors etc. I imagine that style variations might also need to manage their own states like "currentItem".
We are currently piggybacking on the Selectors API to achieve this
My initial thoughts are that this is a bit beyond the scope of the Selectors API and its current use for block support/feature styles. At least as presently implemented.
It was meant as a means of configuring what a CSS selector should be for a given feature/subfeature. The API could be extended though to define what the selectors should be for a given state, element, or style variation.
How best to extend the Selectors API might need some additional thought. I'd be reluctant to have the @currentItem as a key almost equating to a normal feature, it could be confusing or unnecessarily complicate the code.
Perhaps we could add a states property to the block.json selectors object that could be treated as a special case.
{
"selectors": {
"root": ".wp-block-name",
...
"states": {
"current": {
"root": ".wp-block-name.current-item",
...
}
}
}
}Our problem is that we need to be able to define states for elements nested within @currentItem as these may need to be styled differently the standard elements.
Exactly. In addition, the states would also probably need to be scoped by style variation as mentioned above.
Currently this does not work.
This makes sense given the current piggybacking of the Selectors API treats these states as a normal feature. Given they aren't, if you need this functionality now, then it will require its own logic.
Long term, we need to consolidate the generation of style declarations for everything theme.json now covers e.g. the block type itself, block style variations, elements, element pseudo selectors, and features.
Our main question is whether the Selectors API a good choice to use for this feature or should be roll our own custom logic?
I think it makes sense to define the state selectors in the block.json selectors object. So for that, it's a good choice, assuming we tweak the Selectors API accordingly.
Leveraging those selectors into style declarations and ultimately a stylesheet will require logic that handles the discussed scope which is beyond the simple block support features.
So the short answer is; that you'll probably need both 😅
| // Must match @currentItem in block.json. | ||
| $current_item_classname = 'current-menu-item'; |
There was a problem hiding this comment.
The classname contained in the block.json config should be available via the block type, right? Could we not just retrieve that here to ensure the classname is correct?
There was a problem hiding this comment.
Yes we did implement that but it seemed pretty verbose for little benefit.
There was a problem hiding this comment.
No worries, it's certainly a minor implementation detail.
I was only looking at it as enforcing the match as per the inline comment and also as setting an example that others might follow if they're referencing this code.
Something like:
$current_item_classname = wp_get_block_css_selector( $block->block_type, 'states.current' );
|
Great analogy @aaronrobertshaw with variations. "Current" is not a state, it is a "variation" of one part of the block contents. It's like a mini variation. Also, I think the selectors API is very powerful and should be expanded so other parts of the system can use it. In this situation allowing the block author to define what current is (maybe it's the expanded element in a tree, maybe it's the open details element, or maybe the current element in the HTML tree or the aria-current element) - it's a very flexible way to enable this sort of mini variations. Do you think that is problematic as an idea? |
👍 I don't see a lot of difference between what you describe as a "mini variation" and the normal block style variations. They both could define a full set of styles. The difference seems to be only in the scope of their application. A style variation might need its own version of the "mini-variation". Whether the "mini variations" use selectors that can target the root of the block down, or only a certain section within its markup, could be left to the block and its author. For the record, I do still see them as separate within the block.json selectors object though. Could we consider these "mini variations" as "situational variations", or maybe "scenarios"? Naming is hard so feel free to ignore me here. I could almost see "scenario" working for things like "current", "first", "last", "empty", and so on. Back on the block.json selectors config, we could eventually end up with something resembling: There'd be a lot of control on offer there but the block.json files could get rather verbose and bloated. Then again, not every block would need to configure a large number of selectors. 🤔
No arguments here.
No. I didn't mean to come across as against that at all, just express that it would require expanding the API's current scope to be useful here. As we expand the shape of the selectors object, we then also need to update the theme.json processing and util functions like |
|
This is a good discussion and for the record @aaronrobertshaw your input is very much appreciated. I say we implement the above using "scenarios" as the (temporary) key name. We can note this as being TBC, on the condition that we get buy in from a range of contributors before it's merged - we all know how names used in code end up sticking 😅 We'll need a fair amount of refactoring to get this working but it's obviously possible. I believe @draganescu is correct that putting some onus on the block author for implementation is the best way to afford a measure of flexibility and avoiding being too clever in the implementation. |
scruffian
force-pushed
the
add/style-current-menu-item-via-theme-json
branch
from
9596eb7 to
57714c1
Compare
|
Flaky tests detected in e255506. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7872323292
|
|
I did some thinking about this today and I wonder if a better approach would be to make the |
With the ability to style inner elements and block types for extended block style variations coming soon, I think this is a great idea. |
anton-vlasenko
requested review from
tellthemachines and
ajitbohra
as code owners
|
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. |
What?
Implements ability to target the "current menu item" in the Nav block via Theme JSON (only - no UI support).
Addresses part of #42299
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast