Adds current menu class to navigation block #20076
Conversation
draganescu
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
|
Thinking about this PR, if we implement this block in a future |
|
|
||
| $html .= '<li class="' . esc_attr( $css_classes . ( $has_submenu ? ' has-child' : '' ) ) . '"' . $style_attribute . '>' . | ||
| $html .= '<li class="' . esc_attr( $css_classes . ( $has_submenu ? ' has-child' : '' ) ) . | ||
| ( $is_active ? ' current-menu-item' : '' ) . '"' . $style_attribute . '>' . |
There was a problem hiding this comment.
To follow the pattern set by other class names, shouldn't it be something like is-current-navigation-link?
There was a problem hiding this comment.
I think the is- prefix fits pretty good here, in the same way that is-selected, etc... does.
There was a problem hiding this comment.
yes but all WP menus use this convention of current-menu-item, check wp_nav_menu. There is a whole section of default CSS classes and this is-* convention isn't there.
<< I do agree >> with the is-* convention it is just unclear when we start breaking backwards compat.
There was a problem hiding this comment.
The changes made to the Navigation block won't have any effect on existing themes (which don't use blocks at all), so I see no problem with using an is- class here. But maybe someone else has another opinion?
There was a problem hiding this comment.
adding both of them is too much? So, we could guarantee the compatibility with the wp_nav_menu(), and at the same time maybe adding a super basic style for .is-current-navigation-link?
.is-current-navigation-link {
border: 1px solid red; // ;-)
}There was a problem hiding this comment.
Also, it could be handled from the sidebar too, in a similar way that submenus icon does
There was a problem hiding this comment.
@retrofox That sounds like overkill to me. It should be entirely up to the theme to determine if the current navigation link recieves special styles or not.
There was a problem hiding this comment.
Fair enough. Just bringing here the idea that I could be handled in both ways. Through the theme and/or a Navigation option.
|
Could we add a very simple default style? could it be too intrusive? |
draganescu
added
[Block] Navigation
draganescu
force-pushed
the
add/navigation-show-current-page
branch
from
081c57a to
37e20eb
Compare
|
Size Change: -863 B (0%) Total Size: 856 kB
ℹ️ View Unchanged
|
|
This seems to be breaking the experimental Site Editor for me. I'm still not sure if it's related to the template content that I had before. |
Description
Closes #19993
Closes #19858
This PR adds two new updates to the server side rendering of the
Navigationblock:How has this been tested?
2.1 Add a custom CSS class to one of the links
Types of changes
Updated the server side rendering of the
Navigationblock to output the custom CSS class of eachNavigationLinkand to check for the current post ID.