Skip to content

Navigation: Try unifying submenu arrow positioning. #37003

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

Merged
jasmussen merged 4 commits into from
Feb 11, 2022
Merged

Navigation: Try unifying submenu arrow positioning. #37003

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c1601b0
Navigation: Try unifying submenu arrow positioning.
jasmussen Nov 30, 2021
449a60f
Fix click on chevron.
jasmussen Feb 9, 2022
9280e72
Match editor page list markup to front end.
tellthemachines Feb 10, 2022
b090ce8
Remove redundant rule.
jasmussen Feb 11, 2022
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
5 changes: 2 additions & 3 deletions packages/block-library/src/navigation-link/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,15 +215,14 @@ function render_block_core_navigation_link( $attributes, $content, $block ) {
}

$html .= '</span>';
$html .= '</a>';
// End anchor tag content.

if ( isset( $block->context['showSubmenuIcon'] ) && $block->context['showSubmenuIcon'] && $has_submenu ) {
// The submenu icon can be hidden by a CSS rule on the Navigation Block.
$html .= '<span class="wp-block-navigation__submenu-icon">' . block_core_navigation_link_render_submenu_icon() . '</span>';
}

$html .= '</a>';
// End anchor tag content.

if ( $has_submenu ) {
$inner_blocks_html = '';
foreach ( $block->inner_blocks as $inner_block ) {
Expand Down
10 changes: 5 additions & 5 deletions packages/block-library/src/navigation-submenu/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,12 +657,12 @@ export default function NavigationSubmenuEdit( {
/>
</Popover>
) }
{ ( showSubmenuIcon || openSubmenusOnClick ) && (
<span className="wp-block-navigation__submenu-icon">
<ItemSubmenuIcon />
</span>
) }
</ParentElement>
{ ( showSubmenuIcon || openSubmenusOnClick ) && (
<span className="wp-block-navigation__submenu-icon">
<ItemSubmenuIcon />
</span>
) }
<div { ...innerBlocksProps } />
</div>
</Fragment>
Expand Down
4 changes: 2 additions & 2 deletions 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 @@ -245,10 +245,10 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {

$html .= '</span>';

$html .= '<span class="wp-block-navigation__submenu-icon">' . block_core_navigation_submenu_render_submenu_icon() . '</span>';

$html .= '</button>';

$html .= '<span class="wp-block-navigation__submenu-icon">' . block_core_navigation_submenu_render_submenu_icon() . '</span>';

}

if ( $has_submenu ) {
Expand Down
30 changes: 27 additions & 3 deletions packages/block-library/src/navigation/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ $navigation-icon-size: 24px;
align-self: center; // This one affects nested submenu indicators.
line-height: 0;
display: inline-block;
vertical-align: middle;
font-size: inherit;

// Affect the button as well.
Expand All @@ -95,15 +94,18 @@ $navigation-icon-size: 24px;
border: none;

// Scale to font size.
margin-left: 0.25em;
width: 0.6em;
height: 0.6em;
margin-left: 0.25em;
Copy link
Contributor Author

@jasmussen jasmussen Feb 9, 2022

Choose a reason for hiding this comment

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

Well.


svg {
display: inline-block;
stroke: currentColor;
width: inherit;
height: inherit;

// Position the arrow to balance with the the text.
margin-top: 0.075em;
}
}

Expand Down Expand Up @@ -216,6 +218,16 @@ $navigation-icon-size: 24px;
}
}

// Push inwards from right edge of submenu.
.wp-block-navigation__submenu-icon {
margin-right: 0.25em;
}

// For hover-based menus, this class is duplicated for a button.
.wp-block-navigation__submenu-icon .wp-block-navigation__submenu-icon {
Copy link
Contributor

@tellthemachines tellthemachines Feb 10, 2022

Choose a reason for hiding this comment

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

Is this still needed? I notice you removed the extra span here (which wasn't meant to be there, so thanks for fixing 😄 )

Copy link
Contributor Author

@jasmussen jasmussen Feb 11, 2022

Choose a reason for hiding this comment

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

Wildly excellent eye there. I started writing the CSS rule first, then figured the better fix was to remove the span, then forgot to remove the CSS 🙈

Thank you, removed!

margin: 0;
}

// Reset the submenu indicator for horizontal flyouts.
.wp-block-navigation__submenu-icon svg {
transform: rotate(-90deg);
Expand Down Expand Up @@ -245,7 +257,7 @@ $navigation-icon-size: 24px;
}

// Show submenus on click.
.wp-block-navigation-submenu__toggle[aria-expanded="true"] + .wp-block-navigation__submenu-container {
.wp-block-navigation-submenu__toggle[aria-expanded="true"] ~ .wp-block-navigation__submenu-container {
visibility: visible;
overflow: visible;
opacity: 1;
Expand Down Expand Up @@ -300,6 +312,18 @@ button.wp-block-navigation-item__content {
cursor: pointer;
}

// When set to open on click, a button element is used.
// We pad it to include the arrow icon in the clickable area.
// The padding can be blanket for click, since you can't set click and hide the icon.
.wp-block-navigation-item.open-on-click .wp-block-navigation-submenu__toggle {
padding-right: 0.6em + 0.25em; // Same size as icon plus margin.

+ .wp-block-navigation__submenu-icon {
margin-left: -0.6em;
pointer-events: none; // Make the icon inert to allow click on the button.
}
}


/**
* Margins
Expand Down
14 changes: 8 additions & 6 deletions packages/block-library/src/page-list/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,14 +235,16 @@ const PageItems = memo( function PageItems( {

function ItemSubmenuToggle( { title } ) {
return (
<button
className="wp-block-navigation-item__content wp-block-navigation-submenu__toggle"
aria-expanded="false"
>
{ title }
<>
<button
className="wp-block-navigation-item__content wp-block-navigation-submenu__toggle"
aria-expanded="false"
>
{ title }
</button>
<span className="wp-block-page-list__submenu-icon wp-block-navigation__submenu-icon">
<ItemSubmenuIcon />
</span>
</button>
Copy link
Contributor

@tellthemachines tellthemachines Feb 2, 2022

Choose a reason for hiding this comment

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

This change also needs to be done for the page list front end, here. As it stands there's still a diff in the front end between open on hover and click with page list submenus:
Screen Shot 2022-02-02 at 3 02 26 pm
(top one opens on hover, bottom on click).

Copy link
Contributor Author

@jasmussen jasmussen Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you!

I wanted to note that I missed this because it appears I can't currently a navigation menu with only a page list to open submenus on click in the interface. As of #36826, we currently hide those options unless the menu hasSubmenus:
Screenshot 2022-02-02 at 09 45 38

Screenshot 2022-02-02 at 09 44 19

It seems like we should change that to simply just show those options if the menu hasPageList or something along those lines, it doesn't seem worth it to check if the pagelist has submenus. CC: @vcanales

I was able to check the box in the code editor though, so I'll take a stab at addressing the markup issue now.

</>
);
}
6 changes: 3 additions & 3 deletions packages/block-library/src/page-list/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,16 +188,16 @@ function block_core_page_list_render_nested_page_list( $open_submenus_on_click,
$markup .= '<li class="wp-block-pages-list__item' . esc_attr( $css_class ) . '"' . $style_attribute . '>';

if ( isset( $page['children'] ) && $is_navigation_child && $open_submenus_on_click ) {
$markup .= '<button aria-label="' . esc_attr( $aria_label ) . '" class="' . esc_attr( $navigation_child_content_class ) . ' wp-block-navigation-submenu__toggle" aria-expanded="false">' . $title . '<span class="wp-block-page-list__submenu-icon wp-block-navigation__submenu-icon"><svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="none" aria-hidden="true" focusable="false"><path d="M1.50002 4L6.00002 8L10.5 4" stroke-width="1.5"></path></svg></span>' .
'</button>';
$markup .= '<button aria-label="' . esc_attr( $aria_label ) . '" class="' . esc_attr( $navigation_child_content_class ) . ' wp-block-navigation-submenu__toggle" aria-expanded="false">' . esc_html( $title ) .
'</button>' . '<span class="wp-block-page-list__submenu-icon wp-block-navigation__submenu-icon"><svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="none" aria-hidden="true" focusable="false"><path d="M1.50002 4L6.00002 8L10.5 4" stroke-width="1.5"></path></svg></span>';
} else {
$markup .= '<a class="wp-block-pages-list__item__link' . esc_attr( $navigation_child_content_class ) . '" href="' . esc_url( $page['link'] ) . '"' . $aria_current . '>' . $title . '</a>';
}

if ( isset( $page['children'] ) ) {
if ( $is_navigation_child && $show_submenu_icons && ! $open_submenus_on_click ) {
$markup .= '<button aria-label="' . esc_attr( $aria_label ) . '" class="wp-block-navigation__submenu-icon wp-block-navigation-submenu__toggle" aria-expanded="false">';
$markup .= '<span class="wp-block-page-list__submenu-icon wp-block-navigation__submenu-icon"><svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="none" aria-hidden="true" focusable="false"><path d="M1.50002 4L6.00002 8L10.5 4" stroke-width="1.5"></path></svg></span>';
$markup .= '<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="none" aria-hidden="true" focusable="false"><path d="M1.50002 4L6.00002 8L10.5 4" stroke-width="1.5"></path></svg>';
$markup .= '</button>';
}
$markup .= '<ul class="submenu-container';
Expand Down