-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Experiment: Replicate Navigation block using directives #44509
Experiment: Replicate Navigation block using directives #44509
Conversation
Co-authored-by: Mario Santos <SantosGuillamot@users.noreply.github.com> Co-authored-by: David Arenas <david.arenas@automattic.com>
I've tried renaming EDIT: This is what I tried: 13da68a |
When you change the name of that file, it stops being considered an entry point, even when you run Webpack again. It may be related to how entry points are computed. 👀 🤔 |
Size Change: +5.45 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
I've updated the directives to match the latest version on BHE. I've also added some preliminary support for prefix/suffix in the directive name using I added some initial support for modifiers, but I removed it afterward because P/React don't have modifiers, and I'd prefer to first try how it feels to use arrays/objects of options to keep the implementation as simple as possible. |
I added preliminary support for The camelcase renaming of the suffix it's not working here, so we need to think about that as well. Screen.Capture.on.2022-09-29.at.17-53-16.mp4EDIT: I've removed the camelcasing in the suffix for now. |
By the way, for anyone wondering why we are not using arrow functions instead of regular functions, WordPress breaks when it finds the <button wp-class:some-class="({ context }) => context.open"> It should be easy to fix. Although, again, I'm not sure if we should allow this type of JS inlining or not. |
Some of the next steps here:
|
I'm going to try a new API:
Something like this: <div
wp-class="...1..."
wp-class:some-class="...2..."
wp-class:other-class="...3..."
>
...
</div> Will end up in with this object shape: {
"class": {
"default": "...1...",
"some-class": "...2...",
"other-class": "...3..."
}
} This new API makes <div
wp-effect:log-some-value="({ context }) => console.log(context.someValue)"
wp-effect:fetch-something="() => fetch(...)"
>
...
</div> Directives that should only contain one expression can ignore everything but the |
As explained in #44509 (comment)
It's not really needed, we just added previously to test things
I've created a minimalistic implementation of a deep preact signal: Gist, Stackblitz. I'll test it out on Monday 🙂 |
Thanks for the video! Love where the experiment is going
Regarding this: If I am not mistaken, the Btw, we don't need to enqueue the
|
That would be very interesting. Maybe in a different PR? 🙂
True. Fixed, thanks.
I think I tried with a different name starting with I've switched from I had to introduce a Other than that, it's working fine 🙂 |
|
For some reason, it's enqueuing The current size is 9.1Kb. It's not the final size, but it shouldn't grow that much. This already includes: I've refactored it a bit into its own folder. I hope that way can be understood better. I'll try to move the expressions to the |
I've moved the logic to the wpx({
core: {
navigation: {
openMenu: ({ context: { navigation } }) => {
navigation.open = true;
navigation.previousElementWithFocus = window.document.activeElement;
},
closeMenu: ({ context: { navigation } }) => {
navigation.open = false;
},
isMenuOpen: ({ context: { navigation } }) => navigation.open,
isMenuClosed: ({ context: { navigation } }) => !navigation.open,
addFirstElementToContext: ({ context: { navigation }, ref }) => {
navigation.firstMenuElement = ref;
},
focusFirstElement: async ({ context: { navigation }, tick }) => {
if (navigation.open) {
await tick(); // We need to wait until the DOM is updated.
navigation.firstMenuElement.focus();
}
},
focusLastFocusedElement: ({ context: { navigation } }) => {
if (!navigation.open && navigation.previousElementWithFocus)
navigation.previousElementWithFocus.focus();
},
},
},
}); I'm going to stop working on this for now, but feel free to continue adding features, trying different developer experiences or sharing your feedback about all this. |
Regarding this, I decided to publish the pull request where I was testing it, in case it is helpful somehow in the future. As I say there, I just replicated the |
}, | ||
focusFirstElement: async ( { context: { navigation }, tick } ) => { | ||
if ( navigation.open ) { | ||
await tick(); // We need to wait until the DOM is updated. |
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.
This won't be needed once this is merged: preactjs/signals#290
Closing now that the Navigation block is already using directives in Gutenberg. |
What?
Coauthored with @SantosGuillamot and @DAreRodz.
This is a continuation of the #44289 experiment to switch from
micromodal
+ imperative code to declarative directives.Why?
We are experimenting with different hydration mechanisms, and this is one of them. If you're interested in block hydration, please join the Block Hydration Experiments repository 🙂
How?
For now, we have copied the code of the Directives hydration and are trying to copy the functionality of the Alpine directives that @SantosGuillamot used in #44289.
The idea is that Gutenberg could provide all the JS code that's added here. The navigation block would only have to add the directives to the markup in PHP.
This is still very preliminary. We'll keep adding functionality in the next few days.