Correctly apply current-menu-ancestor class in Nav block #67169

getdave · 2024-11-20T15:27:15Z

What?

Applies the current-menu-ancestor class to the <li> not the <a>

Closes #50069

Why?

Consistency with current-menu-item which is applied to the <li>.

How?

Applies the current-menu-ancestor class to the <li> not the <a>

Testing Instructions

You should be able to test this via Playground using the following link

https://playground.wordpress.net/?gutenberg-pr=67169

Note you can install any publicly available Theme for testing using the format:

https://playground.wordpress.net/?gutenberg-pr=67169&theme={theme-slug-here}

On your WordPress test site you can then...

Create Navigation menu
Add a Page
Add a submenu under that page and add some links.
Save
View front of site
Inspect HTML of top level link
See class is on li not a.

Testing Instructions for Keyboard

Screenshots or screencast

getdave · 2024-11-20T15:27:41Z

I'm guessing this class isn't applied in the Editor because there is no concept of this there right?

github-actions · 2024-11-20T15:27:52Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: getdave <[email protected]>
Co-authored-by: MaggieCabrera <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: mrwweb <[email protected]>
Co-authored-by: juanfra <[email protected]>
Co-authored-by: draganescu <[email protected]>
Co-authored-by: bph <[email protected]>
Co-authored-by: jordesign <[email protected]>
Co-authored-by: webmandesign <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

MaggieCabrera · 2024-11-20T16:52:57Z

I'm guessing this class isn't applied in the Editor because there is no concept of this there right?

that sounds about right, it would be hard to map

MaggieCabrera · 2024-11-20T16:53:29Z

packages/block-library/src/navigation-submenu/index.php

@@ -222,7 +222,7 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {

 		if ( strpos( $inner_blocks_html, 'current-menu-item' ) ) {
 			$tag_processor = new WP_HTML_Tag_Processor( $html );
-			while ( $tag_processor->next_tag( array( 'class_name' => 'wp-block-navigation-item__content' ) ) ) {


I wonder how many websites are targeting the "wrong" selector...

Not sure. class is not a first party API though but as we know we need to acknowledge the impact on existing Themes.

How has this been handled in the past? It's not like we can throw an error if someone is using that class.

I think we just consider the risk and go with it. I can't remember when we've done this recently. Maybe a dev note?

@WordPress/block-themers anyone remember an instance of class changing like this?

If I read this right that the .wp-block-navigation-item__content class will be removed by this change, it looks like it's used by a considerable number of themes and a few plugins too.

@mrwweb it looks like that class is only used to iterate there and to determine where to place the current-menu-ancestor class. So, this specific change doesn't mean that the wp-block-navigation-item__content class will be removed.

anyone remember an instance of class changing like this?

I don't.

MaggieCabrera · 2024-11-21T15:50:06Z

I'm ok tentatively approving this if we get some more feedback from themers. If this only affect block themes, I've done a search on a dump I have of block themes in the themes repo and I don't see many themes that would be affected by this change

juanfra

This is testing well for me! I really like that it brings parity with how things work in classic themes.

My fear is also that we don't know how many sites could be targeting the wrong selector. It looks like there's only a handful of themes doing this in the repository, though.

MaggieCabrera · 2024-11-22T09:58:33Z

This is testing well for me! I really like that it brings parity with how things work in classic themes.

My fear is also that we don't know how many sites could be targeting the wrong selector. It looks like there's only a handful of themes doing this in the repository, though.

Yeah, I haven't been able to find much more than that myself, I think this is a low risk change.

MaggieCabrera · 2024-11-22T09:58:52Z

Ugh, sorry I misclicked there :(

getdave · 2024-11-22T13:14:52Z

@bph Would it be worth dropping this in the Outreach channel in Slack for feedback?

bph · 2024-11-22T15:50:24Z

Absolutely! You can also ping the @WordPress/outreach group here on GitHub.

getdave · 2024-11-27T06:16:39Z

I'll leave this open a few more days to allow for further feedback.

Otherwise I think we just need an approving review (with the caveat for the approver that it's agreed that it's not 100% possible to know if this will have some negative side effects).

draganescu

The problem is valid, the code makes sense.

draganescu · 2024-11-27T12:20:43Z

I approved, although I do wonder about it a bit in terms of removing the class from the anchor, what is wrong with:

do not remove the class on the anchor
add a new class to the list item current-menu-item-ancestor

MaggieCabrera

Given how early in the release we are and how little the impact seems to be when inspecting the code for themes in the directory, I think this is good to land as is to get it out and get feedback

getdave · 2024-11-27T13:20:09Z

I approved, although I do wonder about it a bit in terms of removing the class from the anchor, what is wrong with:

do not remove the class on the anchor

add a new class to the list item current-menu-item-ancestor

I don't think we should double up. It could be very confusing. Either move it or leave it "as is".

getdave · 2024-12-02T13:21:07Z

I'm going to merge this one as soon as I can get the PHP unit tests to run correctly.

juanfra · 2024-12-02T15:49:05Z

@getdave I re-ran the failed tests, and they’re all passing now 🚀

…Press#67169 Co-authored-by: getdave <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: mrwweb <[email protected]> Co-authored-by: juanfra <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: bph <[email protected]> Co-authored-by: jordesign <[email protected]> Co-authored-by: webmandesign <[email protected]>

Co-authored-by: getdave <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: mrwweb <[email protected]> Co-authored-by: juanfra <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: bph <[email protected]> Co-authored-by: jordesign <[email protected]> Co-authored-by: webmandesign <[email protected]>

Apply to li not anchor

c35e49e

getdave added [Type] Code Quality Issues or PRs that relate to code quality [Block] Navigation Affects the Navigation Block labels Nov 20, 2024

getdave self-assigned this Nov 20, 2024

getdave requested a review from ajitbohra as a code owner November 20, 2024 15:27

getdave requested review from draganescu and MaggieCabrera November 20, 2024 15:27

MaggieCabrera reviewed Nov 20, 2024

View reviewed changes

juanfra reviewed Nov 22, 2024

View reviewed changes

MaggieCabrera closed this Nov 22, 2024

MaggieCabrera reopened this Nov 22, 2024

getdave added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Nov 22, 2024

getdave mentioned this pull request Nov 27, 2024

HTML structure of navigation blocks differs between editor and front #61170

Open

draganescu approved these changes Nov 27, 2024

View reviewed changes

MaggieCabrera approved these changes Nov 27, 2024

View reviewed changes

carolinan requested a review from cbirdsong November 27, 2024 13:28

getdave merged commit 6561108 into trunk Dec 2, 2024
127 of 145 checks passed

getdave deleted the fix/current-menu-ancestor-class-application branch December 2, 2024 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly apply current-menu-ancestor class in Nav block #67169

Correctly apply current-menu-ancestor class in Nav block #67169

getdave commented Nov 20, 2024 •

edited

Loading

getdave commented Nov 20, 2024

github-actions bot commented Nov 20, 2024 •

edited

Loading

MaggieCabrera commented Nov 20, 2024

MaggieCabrera Nov 20, 2024

getdave Nov 21, 2024

MaggieCabrera Nov 21, 2024

MaggieCabrera Nov 21, 2024

carolinan Nov 21, 2024

mrwweb Nov 21, 2024

juanfra Nov 22, 2024

MaggieCabrera commented Nov 21, 2024

juanfra left a comment

MaggieCabrera commented Nov 22, 2024

MaggieCabrera commented Nov 22, 2024

getdave commented Nov 22, 2024

bph commented Nov 22, 2024

getdave commented Nov 27, 2024

draganescu left a comment

draganescu commented Nov 27, 2024

MaggieCabrera left a comment

getdave commented Nov 27, 2024

getdave commented Dec 2, 2024

juanfra commented Dec 2, 2024

Correctly apply current-menu-ancestor class in Nav block #67169

Correctly apply current-menu-ancestor class in Nav block #67169

Conversation

getdave commented Nov 20, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

getdave commented Nov 20, 2024

github-actions bot commented Nov 20, 2024 • edited Loading

MaggieCabrera commented Nov 20, 2024

MaggieCabrera Nov 20, 2024

Choose a reason for hiding this comment

getdave Nov 21, 2024

Choose a reason for hiding this comment

MaggieCabrera Nov 21, 2024

Choose a reason for hiding this comment

MaggieCabrera Nov 21, 2024

Choose a reason for hiding this comment

carolinan Nov 21, 2024

Choose a reason for hiding this comment

mrwweb Nov 21, 2024

Choose a reason for hiding this comment

juanfra Nov 22, 2024

Choose a reason for hiding this comment

MaggieCabrera commented Nov 21, 2024

juanfra left a comment

Choose a reason for hiding this comment

MaggieCabrera commented Nov 22, 2024

MaggieCabrera commented Nov 22, 2024

getdave commented Nov 22, 2024

bph commented Nov 22, 2024

getdave commented Nov 27, 2024

draganescu left a comment

Choose a reason for hiding this comment

draganescu commented Nov 27, 2024

MaggieCabrera left a comment

Choose a reason for hiding this comment

getdave commented Nov 27, 2024

getdave commented Dec 2, 2024

juanfra commented Dec 2, 2024

getdave commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024 •

edited

Loading