-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Experimental: Navigation block with the Interactivity API #50041
Conversation
Co-authored-by: David Arenas <david.arenas@automattic.com>
This is still pretty basic, just to check that it works. Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Size Change: 0 B Total Size: 1.39 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it for a spin, and the version built on top of Interactivity API seems to work the same as the old one. That's impressive, given that the focus is managed manually 😄
I was a bit surprised that the existing implementation of the overlay doesn't handle clicking outside of the menu as closing the dialog. Another missing feature was the using arrow up/down to move the focus between items in the submenu. Anyway, taking all that into account, it seems like both implementations have the same set of features.
See the recorded screencast with the existing behavior in the block editor when using menus and dialogs:
Screen.Recording.2023-04-25.at.13.21.56.mov
I'm not saying we need to match that on the front end, but it is something I found surprising.
Flaky tests detected in 4367b4d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4935185137
|
I think I resolved both issues reported earlier:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is now in good shape. I would appreciate some help with testing my recent changes.
Test failures from CI jobs for checking PHP unit tests fail now because of the regression introduced in |
Taking a look at them, @gziolo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Tested and worked as expected. Keyboard navigation works smoothly.
Congrats!
@gziolo, I've tested the changes you mentioned in #50041 (comment), and the issues seem to be solved now. 👍 |
…h-interactivity-api
) ) { | ||
$w->set_attribute( 'data-wp-class.is-menu-open', 'context.core.navigation.isMenuOpen' ); | ||
$w->set_attribute( 'data-wp-bind.aria-hidden', '!context.core.navigation.isMenuOpen' ); | ||
$w->set_attribute( 'data-wp-effect', 'effects.core.navigation.initModal' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SantosGuillamot, would it make sense to rename the effect to reflect the fact it works with the menu when there is no modal, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it to initMenu
in this commit.
Version 15.9.20230511 returns 404 for the following files:
|
It's very likely that those files aren't whitelisted for inclusion in the zip file attached to the release: gutenberg/bin/build-plugin-zip.sh Lines 81 to 89 in 37d9839
|
I opened #50598 to fix the issue with missing scripts. |
What?
This pull request adds an experimental option to toggle between the Navigation block using the Interactivity API and the old scripts that use
micromodal
:I recorded this video explaining in detail the purpose of the pull request and how it has been implemented:
https://www.loom.com/share/c07afacb4e464de8a8e7b253930dcd1b
Why?
This pull request is part of the proposal to use the Interactivity API as a standard for the blocks frontend. The main goal is to start testing real blocks with this API, which could also serve as an example, and see what they would look like.
In terms of performance, it is true that, if you only use the Interactivity API for the navigation block, it sends a few more kBs to the client than the current approach (12kBs vs 4kBs). However, if more blocks use the Interactivity API, they would reuse those 12kBs and would result in fewer kBs overall. There are already ongoing pull requests that aim to use this code like the Lightbox in images. And there could be more core blocks in the future using it.
Additionally, this would prevent the block from depending on external libraries like
micromodal
.How?
micromodal
is doing.There are several options in the navigation block. These are the use cases taken into account to replicate the current behavior:
I tried to cover all the use cases, but let me know if you are missing anything.
Testing
Basically, we have to ensure that the Navigation block, the navigation submenu, and the page list behave exactly the same when the option is activated/deactivated.
As part of this pull request, I added e2e tests for the navigation to check all those user experiences mentioned previously. With both options tests are passing, but maybe there are more use cases not contemplated there.