Repository navigation
Nav submenu: add support to style current submenu in theme.json - #76367
MaggieCabrera wants to merge 7 commits into
Conversation
MaggieCabrera
changed the title
|
Flaky tests detected in d352ea8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29909695663
|
c11d8b5 to
40b2cd6
Compare
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
MaggieCabrera
added
[Type] Enhancement
|
I'm a bit hesitant about this one. I think we should be moving in the direction of making navigation link and navigation submenu blocks simply variations of the same block. This will make that harder to do and moves us away from that direction. If this is a feature that we think themes will use then i might be a good compromise, however I'm struggling to think of a use case for it... |
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
My main question is can you target the top level menu item represented by a submenu if it is current? I'm conscious it's a different block that core/navigation-link but you'd still want to be able to style the parent.
Styling menus within the submenu dropdown seems like a real edge case. Not sure if this is required but I'd defer to your theme expertise on that.
I've never personally needed to. That said, I don't think this PR is a must, but it certainly offers more flexibility at very little cost. I would love to hear what @WordPress/block-themers have to say about this. |
Yes, the navigation-link selector also covers navigation-submenu links so it works in trunk already. |
github-project-automation
Bot
moved this to 🔎 Needs Review
in WordPress 7.1 Editor Tasks
|
I would like to check the status of this PR. Does this PR need to be shipped for 7.1? It appears that this PR is not included in the core backport that has already been committed. Therefore, if we are to move forward with this PR, we will need to create a new core backport anyway. |
I think we can remove it from 7.1, it's not clear that we even need/want it |
t-hamano
moved this from 🔎 Needs Review
to 🦵 Punted to 7.2
in WordPress 7.1 Editor Tasks
|
7.2 or when it happens is fine. But wanted to comment on this:
In case you're referring to the ability to style the Then I could delete the following from my stylesheet: |
|
+1 for bringing this in, I've got this request multiple times in the past. It's just a small enhancement but I think it has a much bigger impact. |
I agree that it could make things harder. The otherside of the coin is that this is a feature that users/theme authors need now. Delaying it based on a hypothetical merging of the two blocks wouldn't feel right given the only opposition is a technical one. Let's deliver user value now. I would prefer that we consider and design a migration path that allows any styles defined against Submenu to be migrated in the scenario that the Submenu block is normalized with the standard Nav Link block. @MaggieCabrera do you think that is possible? Can you outline a potential migration route?
Update: I realised that Submenu "current" styling is already handled. This PR is for the children of the submenu blocks. I think we should bring this in. |
8fd42d5 to
c360e7b
Compare
getdave
added
the
[Feature] Navigation Menus
|
@MaggieCabrera I rebased and updated to match the new syntax for "currrent". |
github-project-automation
Bot
moved this to 🔎 Needs Review
in WordPress 7.2 Editor Tasks
d352ea8 to
dc7d6e9
Compare
🤖 PR meta 🤖🎉 PropsIf 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. Updated as activity occurs, without notifying anyone named here. Add the 📦 Bundle sizeSize Change: +35 B (0%) Total Size: 8.96 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. Regression: updating a specific option will only change its value and will not corrupt others in
|
Keep VALID_BLOCK_PSEUDO_SELECTORS in sync with the PHP constant, so `:hover`, `:focus`, `:focus-visible` and `:active` styles on core/navigation-submenu show in the editor as well as on the front end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 7.1 backport (wordpress-develop#11174) was committed without this change, so it now has its own 7.2 Core PR, wordpress-develop#14284. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Would love reviews on this one, @WordPress/gutenberg-core |
mikachan
left a comment
mikachan
left a comment
There was a problem hiding this comment.
Thanks for this! Left a few inline comments with a couple of things that I think need fixing before we bring this in.
| "shadow": ".wp-block-navigation-submenu > .wp-block-navigation__submenu-container" | ||
| "shadow": ".wp-block-navigation-submenu > .wp-block-navigation__submenu-container", | ||
| "states": { | ||
| "-current": ".wp-block-navigation .wp-block-navigation-submenu .current-menu-item" |
There was a problem hiding this comment.
I think this selector matches the wrong element. When a submenu is the current page, navigation-submenu/index.php (~L131) puts current-menu-item on the submenu's own wrapper <li>, next to wp-block-navigation-submenu. Because there's a space between .wp-block-navigation-submenu and .current-menu-item, this selector only matches current items inside a submenu. In practice that means a current Navigation Link nested in a dropdown, not the current submenu itself.
So with the theme.json from the testing instructions, the green -current submenu style would override the pink -current link style for any current link inside a dropdown. A current top-level submenu would stay pink, because it still matches the link selector .wp-block-navigation .current-menu-item.
Should this be a compound selector instead?
"-current": ".wp-block-navigation .wp-block-navigation-submenu.current-menu-item"The README line and the expected CSS in the test would need the same change.
| "core/navigation-submenu": { | ||
| "$ref": "#/definitions/stylesPropertiesAndElementsComplete" | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/definitions/stylesPropertiesAndElementsComplete" | ||
| }, | ||
| { | ||
| "$ref": "#/definitions/stylesBlocksPseudoSelectorsProperties" | ||
| }, | ||
| { | ||
| "$ref": "#/definitions/stylesBlocksCustomStatesProperties" | ||
| } | ||
| ] |
There was a problem hiding this comment.
I don't think this allOf can validate -current or :hover. stylesPropertiesAndElementsComplete has a propertyNames constraint that only allows style property names, elements, variations and breakpoint keys. Every branch of an allOf has to pass, so that constraint will reject the new keys even though the other two $refs define them.
This means the testing instructions snippet would get flagged in editors that use the schema. Could this copy the core/navigation-link definition just above? That one builds from stylesProperties plus the pseudo, custom-state and responsive-with-pseudo definitions, and its own propertyNames anyOf includes stylesBlocksPseudoSelectorsPropertyNames and stylesBlocksCustomStatesPropertyNames.
| "shadow": ".wp-block-navigation-submenu > .wp-block-navigation__submenu-container" | ||
| "shadow": ".wp-block-navigation-submenu > .wp-block-navigation__submenu-container", | ||
| "states": { | ||
| "-current": ".wp-block-navigation .wp-block-navigation-submenu .current-menu-item" |
There was a problem hiding this comment.
Related to the pseudo-selector question: once the selector targets the submenu <li>, -current → color.text will also be inherited by every link in that submenu's dropdown. Navigation Link avoids this because it has no children. Is that acceptable? Or should -current target the submenu's own > .wp-block-navigation-item__content instead?
|
Moving this out of 7.2, it needs more work |
…on`. This changeset backports the `WP_Theme_JSON` changes from GB76367, and is a follow-up to #64806. Themes can style the current link inside a Navigation Submenu dropdown with `-current` on `core/navigation-submenu` in `theme.json`, including its `:hover`, `:focus`, `:focus-visible` and `:active states`. The block's `selectors.states` selector arrives with the Gutenberg package sync, so the new test uses `get_styles_for_block()` with explicit style nodes, like the existing custom state tests. Developed in: - #14284 - WordPress/gutenberg#76367 Follow-up to #62641. Props onemaggie. Fixes #66312. See #64806. git-svn-id: https://develop.svn.wordpress.org/trunk@64600 602fd350-edb4-49c9-b593-d223f7449a82
…on`. This changeset backports the `WP_Theme_JSON` changes from GB76367, and is a follow-up to #64806. Themes can style the current link inside a Navigation Submenu dropdown with `-current` on `core/navigation-submenu` in `theme.json`, including its `:hover`, `:focus`, `:focus-visible` and `:active states`. The block's `selectors.states` selector arrives with the Gutenberg package sync, so the new test uses `get_styles_for_block()` with explicit style nodes, like the existing custom state tests. Developed in: - WordPress/wordpress-develop#14284 - WordPress/gutenberg#76367 Follow-up to #62641. Props onemaggie. Fixes #66312. See #64806. Built from https://develop.svn.wordpress.org/trunk@64600 git-svn-id: http://core.svn.wordpress.org/trunk@63750 1a063a9b-81f0-0310-95a4-ce76da25c4cd
What?
Follow up to #75736
Following the discussion on the other PR, we are adding support for
currentstate to the submenu block tooWhy?
For consistency and to allow users more nunaced control over how they style their navigation blocks.
How?
We are adding support the same we did for the nav link, by adding it to the allowed blocks constant, adding a selector on block.json and modifying the schema andadding tests.
Testing Instructions
Add this to your theme.json, before this PR only the nav-link applied, now we can target submenus specifically.
Screenshots or screencast