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

Experiment: Replicate Navigation block using directives (Alpinejs) #44289

Closed
wants to merge 10 commits into from
Closed

Experiment: Replicate Navigation block using directives (Alpinejs) #44289

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7de6de2
Merge branch 'trunk' into navigation-block-with-directives
SantosGuillamot Sep 19, 2022
cc19f6a
Replace micromodal for Alpinejs directives
SantosGuillamot Sep 20, 2022
c44fbb7
Remove micromodal attributes
SantosGuillamot Sep 20, 2022
c406a3d
Focus first element of the menu
SantosGuillamot Sep 20, 2022
473f140
Go back to hamburger button when closing modal
SantosGuillamot Sep 21, 2022
a134608
Remove scripts versions to prevent redirections
SantosGuillamot Sep 21, 2022
0a0598f
Move focus logic to x-effect
luisherranz Sep 21, 2022
b4ebb34
Change view.js in navigation block for directives
SantosGuillamot Sep 22, 2022
31871e8
Fix linting errors
SantosGuillamot Sep 23, 2022
df54ee4
Fix more linting errors
SantosGuillamot Sep 23, 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
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@
}
}
},
"viewScript": [ "file:./view.min.js", "file:./view-modal.min.js" ],
"viewScript": "file:./view.min.js",
"editorStyle": "wp-block-navigation-editor",
"style": "wp-block-navigation"
}
43 changes: 32 additions & 11 deletions packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,10 +426,25 @@ function render_block_core_navigation( $attributes, $content, $block ) {
wp_enqueue_script( 'wp-block-navigation-view' );
}

$should_load_modal_view_script = isset( $attributes['overlayMenu'] ) && 'never' !== $attributes['overlayMenu'];
if ( $should_load_modal_view_script ) {
wp_enqueue_script( 'wp-block-navigation-view-modal' );
}
// Register @alpine/focus plugin
wp_register_script(
'AlpineJS_focus',
'https://unpkg.com/@alpinejs/focus@3.10.3/dist/cdn.min.js',
array(),
"3.10.3",
luisherranz marked this conversation as resolved.
Show resolved Hide resolved
true // Load it in the footer
);
wp_enqueue_script( 'AlpineJS_focus' );

// Register Alpine
wp_register_script(
'AlpineJS',
'https://unpkg.com/alpinejs@3.10.3/dist/cdn.min.js',
array('AlpineJS_focus'),
"3.10.3",
luisherranz marked this conversation as resolved.
Show resolved Hide resolved
true // Load it in the footer
);
wp_enqueue_script( 'AlpineJS' );

$inner_blocks = $block->inner_blocks;

Expand Down Expand Up @@ -559,7 +574,7 @@ function render_block_core_navigation( $attributes, $content, $block ) {
foreach ( $inner_blocks as $inner_block ) {
if ( ( 'core/navigation-link' === $inner_block->name || 'core/home-link' === $inner_block->name || 'core/site-title' === $inner_block->name || 'core/site-logo' === $inner_block->name || 'core/navigation-submenu' === $inner_block->name ) && ! $is_list_open ) {
$is_list_open = true;
$inner_blocks_html .= '<ul class="wp-block-navigation__container">';
$inner_blocks_html .= '<ul x-ref="items" class="wp-block-navigation__container">';
}
if ( 'core/navigation-link' !== $inner_block->name && 'core/home-link' !== $inner_block->name && 'core/site-title' !== $inner_block->name && 'core/site-logo' !== $inner_block->name && 'core/navigation-submenu' !== $inner_block->name && $is_list_open ) {
$is_list_open = false;
Expand Down Expand Up @@ -632,11 +647,17 @@ function render_block_core_navigation( $attributes, $content, $block ) {
$toggle_aria_label_close = $should_display_icon_label ? 'aria-label="' . __( 'Close menu' ) . '"' : ''; // Close button label.

$responsive_container_markup = sprintf(
'<button aria-haspopup="true" %3$s class="%6$s" data-micromodal-trigger="%1$s">%9$s</button>
<div class="%5$s" style="%7$s" id="%1$s">
<div class="wp-block-navigation__responsive-close" tabindex="-1" data-micromodal-close>
<div class="wp-block-navigation__responsive-dialog" aria-label="%8$s">
<button %4$s data-micromodal-close class="wp-block-navigation__responsive-container-close">%10$s</button>
'<button x-on:click="open = true; $focus.within($refs.items).first()" aria-haspopup="true" %3$s class="%6$s">%9$s</button>
Copy link
Member

Choose a reason for hiding this comment

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

A few things here with the $focus.within($refs.items):

  • The $focus.within($refs.items).first() part is not triggered when you focus on the hamburguer menu and use Enter to open the menu.

    I wonder what directive/pattern we could use here to take into account both the click and the keyboard. Maybe a simple x-effect that reacts to open === true to make it more declarative?

  • $focus.within($refs.items).first() is focusing the last element to me! I have no idea why.

    ezgif-4-d303afd612.mp4

  • On close, the micromodal version focus the hamburguer menu (shown in video above). We should include that.

  • What do you think about the $focus.within() API. Was it easy to understand? Would it be an easier way to deal with the focus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • The $focus.within($refs.items).first() part is not triggered when you focus on the hamburguer menu and use Enter to open the menu.

I wonder what directive/pattern we could use here to take into account both the click and the keyboard. Maybe a simple x-effect that reacts to open === true to make it more declarative?

  • $focus.within($refs.items).first() is focusing the last element to me! I have no idea why.

I'm not sure about these two points. It seems to be working for me. Or am I doing something wrong?

https://www.loom.com/share/a144d46e28e6403ca77e2fa7674c87f2

On close, the micromodal version focus the hamburguer menu (shown in video above). We should include that.

Regarding this, I believed it was working 🤷 I have just pushed a commit fixing it using x-ref and $focus again. Not sure if it is the best way though, maybe we could use just something like document.querySelector(".hamburger").focus().

What do you think about the $focus.within() API. Was it easy to understand? Would it be an easier way to deal with the focus?

For me, it was easy to understand but maybe it was just because there is an example really similar to this use case in the docs. If that wasn't the case, I would have probably struggled to understand it.

Regarding if there is a better way to handle the focus, maybe for these cases using the directives is not 100% necessary and we could use just document.querySelector(".hamburger").focus() as mentioned above.

Anyway, the functions of the $focus seem easy to use to me and cover many use cases.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about these two points. It seems to be working for me. Or am I doing something wrong?

Oh, you're right. Alpine uses tappable, and the list of focusable elements is not the same as the one in micromodal, so maybe that's why. Don't worry about it. It's not important.

I wonder what directive/pattern we could use here to take into account both the click and the keyboard. Maybe a simple x-effect that reacts to open === true to make it more declarative?

I've moved the focus to an x-effect directive (which is similar to React's useEffect) in 0a0598f to make it more declarative. It seems to work fine, but could you please check it out?

x-effect="open === true ? $focus.within($refs.items).first() : $focus.focus($refs.hamburger)

I couldn't use if ... else inside the x-effect. I'm not sure if I was doing something wrong.

Not sure if it is the best way though, maybe we could use just something like document.querySelector(".hamburger").focus().

Good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved the focus to an x-effect directive (which is similar to React's useEffect) in 0a0598f to make it more declarative. It seems to work fine, but could you please check it out?

Oh, I missed the x-effect comment sorry. It seems clearer this way. However, if I am not mistaken when the page is loaded open = false so it leads to focusing on the hamburger even if the user hasn't interacted with the menu. I assume this shouldn't be the case right? The only thing I can think of to solve it is to change the initial state of open to undefined or an empty string and change the conditional to only focus on true /false:

x-effect="if (open === true) { $focus.within($refs.items).first() } else if (open === false) { $focus.focus($refs.hamburger) }"

This way it seems to work for me.

And using the if ... else is working for me 🤷

Copy link
Member

Choose a reason for hiding this comment

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

However, if I am not mistaken when the page is loaded open = false so it leads to focusing on the hamburger even if the user hasn't interacted with the menu

Oh, true. That's a bummer.

Copy link
Member

@luisherranz luisherranz Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

$focus.within($refs.items).first() is focusing the last element to me! I have no idea why.

I think I've discovered why. One of the items has tabindex="0" in the HTML. I don't know where did it come from.

<a
  class="wp-block-navigation-item__content"
  href="http://localhost:8888/eius-magnam-sed-laboriosam-reprehenderit-exercitationem/"
  tabindex="0"
>
  <span class="wp-block-navigation-item__label">Eius magnam</span>
</a>

So it is not a problem for this experiment, don't worry 🙂

<div
x-on:keydown.escape="open = false"
:class="open && \'is-menu-open\ has-modal-open\'"
:aria-hidden="open ? false : true"
class="%5$s" style="%7$s"
id="%1$s"
>
<div class="wp-block-navigation__responsive-close" tabindex="-1">
<div x-trap="open" class="wp-block-navigation__responsive-dialog" aria-label="%8$s" aria-modal="true" role="dialog">
<button x-on:click="open = false" %4$s class="wp-block-navigation__responsive-container-close">%10$s</button>
<div class="wp-block-navigation__responsive-container-content" id="%1$s-content">
%2$s
</div>
Expand All @@ -656,7 +677,7 @@ function render_block_core_navigation( $attributes, $content, $block ) {
);

return sprintf(
'<nav %1$s>%2$s</nav>',
'<nav %1$s x-data="{open: false}">%2$s</nav>',
$wrapper_attributes,
$responsive_container_markup
);
Expand Down
68 changes: 0 additions & 68 deletions packages/block-library/src/navigation/view-modal.js
View file
Open in desktop

This file was deleted.