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.

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

Adds current menu class to navigation block #20076

Open
wants to merge 2 commits into
base: master
from

Conversation

@draganescu
Copy link
Contributor

draganescu commented Feb 6, 2020

Description

Closes #19993
Closes #19858

This PR adds two new updates to the server side rendering of the Navigation block:

  • it renders on each navigation link the custom CSS class saved from the block editor
  • it renders on navigation links a class named 'current-menu-item'

How has this been tested?

  1. In a post add a new navigation block
  2. Add three links to the post you are editing and two other posts and/or pages
    2.1 Add a custom CSS class to one of the links
  3. Save the block as a reusable block
  4. Edit the pages/posts that you linked to and add the reusable block
  5. Visit the pages and notice the class being added to the current menu item and the custom CSS class being rendered

Types of changes

Updated the server side rendering of the Navigation block to output the custom CSS class of each NavigationLink and to check for the current post ID.

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Feb 6, 2020

Thinking about this PR, if we implement this block in a future menus.php admin page, the Navigation block's rendering should handle all the classes printed by wp_nav_menu so things don't break, so this 'current-menu-item' does not seem sufficient.


$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 . '>' .

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Feb 6, 2020

Contributor

To follow the pattern set by other class names, shouldn't it be something like is-current-navigation-link?

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 6, 2020

Contributor

I think the is- prefix fits pretty good here, in the same way that is-selected, etc... does.

This comment has been minimized.

Copy link
@draganescu

draganescu Feb 7, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Feb 7, 2020

Contributor

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?

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 7, 2020

Contributor

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; // ;-)
}

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 7, 2020

Contributor

Also, it could be handled from the sidebar too, in a similar way that submenus icon does

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Feb 7, 2020

Contributor

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

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 7, 2020

Contributor

Fair enough. Just bringing here the idea that I could be handled in both ways. Through the theme and/or a Navigation option.

@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Feb 6, 2020

Could we add a very simple default style? could it be too intrusive?

@ellatrix ellatrix removed this from the Gutenberg 7.5 milestone Feb 10, 2020
@draganescu draganescu added this to 👀 PRs to review in Navigation block via automation Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Navigation block
  
👀 PRs to review
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.