Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Dec 2, 2024
+1 −1
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation-submenu/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #67134

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

while ( $tag_processor->next_tag( array( 'class_name' => 'wp-block-navigation-item' ) ) ) {
$tag_processor->add_class( 'current-menu-ancestor' );
}
$html = $tag_processor->get_updated_html();
Expand Down
Loading