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

Jump to bottom

[WIP] Navigation Menu: dropdown activation mode #18445

Closed
wants to merge 6 commits into from
Closed

[WIP] Navigation Menu: dropdown activation mode #18445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ce26bee
navigation-menu: add `menuEventActivation` attr
retrofox Nov 11, 2019
92b346b
navigation-menu: add MenuActivation insp control
retrofox Nov 11, 2019
0fb2b72
navigation-menu: add menu action to inspector
retrofox Nov 11, 2019
bad7457
navigation-menu: init value for menu activation
retrofox Nov 11, 2019
eade6e5
navigation-menu: handling `hover` event
retrofox Nov 11, 2019
bc939b1
navigation-menu: experiment onClick hac
retrofox Nov 12, 2019
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
14 changes: 10 additions & 4 deletions packages/block-library/src/navigation-menu/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { __ } from '@wordpress/i18n';
import useBlockNavigator from './use-block-navigator';
import BlockNavigationList from './block-navigation-list';
import BlockColorsStyleSelector from './block-colors-selector';
import MenuActivation from './inspector-control-menu-activation';

function NavigationMenu( {
attributes,
Expand Down Expand Up @@ -73,7 +74,7 @@ function NavigationMenu( {
return null;
}

return pages.map( ( { title, type, link: url, id } ) => (
return pages.map( ( { title, type, link: url, id, menuEventActivation } ) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering whether menuEventActivation would be clearer as dropdownActivationEvent. At the least swapping Event and Activation should happen to make the "thing" we are describing (ie: the event) is clearer (ie: menuActivationEvent).

createBlock( 'core/navigation-menu-item',
{
type,
Expand All @@ -82,6 +83,7 @@ function NavigationMenu( {
label: title.rendered,
title: title.raw,
opensInNewTab: false,
menuEventActivation,
}
)
) );
Expand Down Expand Up @@ -226,11 +228,15 @@ function NavigationMenu( {
/>
</PanelBody>
) }
<PanelBody
title={ __( 'Navigation Structure' ) }
>
<PanelBody title={ __( 'Navigation Structure' ) }>
<BlockNavigationList clientId={ clientId } />
</PanelBody>

<MenuActivation
selected={ attributes.menuEventActivation }
onChange={ ( menuEventActivation ) => setAttributes( { menuEventActivation } ) }
/>

</InspectorControls>

<div className={ navigationMenuClasses } style={ navigationMenuInlineStyles }>
Expand Down
39 changes: 35 additions & 4 deletions packages/block-library/src/navigation-menu/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ function render_block_navigation_menu( $attributes, $content, $block ) {

$colors = build_css_colors( $attributes );

return "<nav class='wp-block-navigation-menu' {$comp_inline_styles}>" .
$menu_activation_css_class = ! empty( $attributes['menuEventActivation'] ) && 'onHover' === $attributes['menuEventActivation']
? 'is-activated-by-hover'
: 'is-activated-by-click';

return "<nav class='wp-block-navigation-menu {$menu_activation_css_class}' {$comp_inline_styles}>" .
build_navigation_menu_html( $block, $colors ) .
'</nav>';
}
Expand All @@ -94,10 +98,29 @@ function render_block_navigation_menu( $attributes, $content, $block ) {
* @return string Returns an HTML list from innerBlocks.
*/
function build_navigation_menu_html( $block, $colors ) {
$on_hover_mode = ! empty( $attributes['menuEventActivation'] ) && 'onHover' === $attributes['menuEventActivation'];

$html = '';
$level = 0;
foreach ( (array) $block['innerBlocks'] as $key => $menu_item ) {
$has_sub_items = count( (array) $menu_item['innerBlocks'] ) > 0;

$html .= '<li class="wp-block-navigation-menu-item ' . $colors['bg_css_classes'] . '"' . $colors['bg_inline_styles'] . '>' .
(
( $has_sub_items && ! $on_hover_mode )
? (
'<input
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need some ARIA roles here such as aria-pressed and aria-expanded.

id="toggle-handler-' . $level . '-' . $key . '"
class="wp-block-navigation-menu-item__toggle-handler"
type="checkbox"
>' .
'<label
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps take a look at Bootstrap's "Dropdown" implementation for a11y guidance here.

for="toggle-handler-' . $level . '-' . $key . '"
class="wp-block-navigation-menu-item__toggle-for"
>'
)
: ''
) .
'<a
class="wp-block-navigation-menu-item__link ' . $colors['text_css_classes'] . '"
' . $colors['text_inline_styles'];
Expand All @@ -113,18 +136,21 @@ class="wp-block-navigation-menu-item__link ' . $colors['text_css_classes'] . '"
if ( isset( $menu_item['attrs']['opensInNewTab'] ) && true === $menu_item['attrs']['opensInNewTab'] ) {
$html .= ' target="_blank" ';
}
// End appending HTML attributes to anchor tag.

// Start anchor tag content.
$html .= '>';

if ( isset( $menu_item['attrs']['label'] ) ) {
$html .= $menu_item['attrs']['label'];
}
$html .= '</a>';
// End anchor tag content.

if ( count( (array) $menu_item['innerBlocks'] ) > 0 ) {
$html .= build_navigation_menu_html( $menu_item, $colors );
$html .= ( $has_sub_items && ! $on_hover_mode ) ? '</label>' : '';

if ( $has_sub_items ) {
$level = $level + 1;
$html .= build_navigation_menu_html( $menu_item, $colors, $level );
}

$html .= '</li>';
Expand Down Expand Up @@ -185,6 +211,11 @@ function register_block_core_navigation_menu() {
'textColorCSSClass' => array(
'type' => 'string',
),

'menuEventActivation' => array(
'type' => 'string',
'default' => 'onHover',
),
),

'render_callback' => 'render_block_navigation_menu',
Expand Down
32 changes: 32 additions & 0 deletions packages/block-library/src/navigation-menu/inspector-control-menu-activation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* External dependencies
*/

/**
* WordPress dependencies
*/
import { RadioControl, PanelBody } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

export default ( { selected, onChange } ) => {
return (
<PanelBody title={ __( 'Dropdown Menu Activation' ) }>
<RadioControl
options={ [
{
label: __('Hover / Focus Event.' ),
value: 'onHover',
},
{
label: __('Click / Tap Event.' ),
value: 'onClick',
}
] }
selected={ selected }
onChange={ onChange }
label={ __( 'Dropdown menu activation' ) }
help={ __( 'Select the way of how the dropdown menu will be activated.' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Select the way of how the dropdown menu will be activated

Could be phrased as

Choose the way dropdown menus will be activated

/>
</PanelBody>
)
};
54 changes: 38 additions & 16 deletions packages/block-library/src/navigation-menu/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,6 @@
li {
position: relative;
z-index: 1;

&:hover,
&:focus-within {
cursor: pointer;
z-index: 99999;
}

// Submenu Display
&:hover > ul,
&:focus-within > ul,
& ul:hover,
& ul:focus {
visibility: visible;
opacity: 1;
display: block;
}
}

& > li {
Expand Down Expand Up @@ -106,4 +90,42 @@
}
}

// Menu event activation.
&.is-activated-by-hover li {
&:hover,
&:focus-within {
cursor: pointer;
z-index: 99999;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this super high value about? 😄

}

// Submenu Display
&:hover > ul,
&:focus-within > ul,
& ul:hover,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these needed? If the ul is within the li then the :hover on the li should sustain the hover effect.

& ul:focus {
visibility: visible;
opacity: 1;
display: block;
}
}

&.is-activated-by-click li {
input.wp-block-navigation-menu-item__toggle-handler {
display: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make the control impossible to access for assistive tech. Better to use the VisuallyHidden component or a CSS solution to hide visually but retain access for assistive tech:

See also https://wordpress.slack.com/archives/C02QB2JS7/p1573637389298100

}

label.wp-block-navigation-menu-item__toggle-for {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid qualifying with label unless it is required for specificity. Otherwise we're increasing the specificity for no reason which I would tend to avoid at all costs.

Ditto for input. above.

font-size: inherit;
display: block;
padding: 0;
margin: 0;
cursor: pointer;
}

input.wp-block-navigation-menu-item__toggle-handler:checked + label.wp-block-navigation-menu-item__toggle-for + ul {
visibility: visible;
opacity: 1;
display: block;
}
}
}