-
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
Interactivity API: Add wp-data-on-window
and wp-data-on-document
directives
#57931
Conversation
Size Change: +395 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
@@ -8,7 +8,7 @@ import { deepSignal, peek } from 'deepsignal'; | |||
* Internal dependencies | |||
*/ | |||
import { createPortal } from './portals'; | |||
import { useWatch, useInit } from './utils'; | |||
import { useWatch, useInit, useEffect } from './utils'; |
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.
If you're going to use useEffect
from @wordpress/interactivity
, you can use useInit
instead.
@DAreRodz: or what do we do? Do we keep using useWatch
and useInit
in the directives? Or do we switch back to hooks that don't have an unnecessary call to withScope
? To me, it feels weird to have that double set of the scope.
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.
If there are cases in which we need useWatch
and useInit
. It may be worth the cost of that withScope
and simplify all the directives with one solution.
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.
Yeah, I'm still thinking about what approach is better. I would prefer the same hooks for directives and store callbacks, but I agree that double-setting the same scope feels weird. 🤔
Let's consider the DevX.
- If we stay with the same hooks and
evaluate
, everything is transparent for the developer (although internally weird things happen). - If we have multiple hook versions, we would have to clarify which one is used in which case. And, if the API is meant to be publicly exposed from
@wordpress/interactivity
, we should use different names. E.g.useEffect
anduseScopedEffect
. And maybe you could have more complex cases where you need to use a scoped hook and callevaluate()
inside, leading to the same weirdness as the first option (I haven't thought of any, but who knows). - We can also get rid of the scoped versions and expose a
withScope()
function (maybe even forevaluate
) so devs explicitly decide when the scope is available.withScope( evaluate )( entry ); useEffect( withScope( () => { // Do something. } );
Any other ideas? 😄
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'm just talking about our directives.js
file.
Replace this:
import { useInit } from "..";
// ...
// data-wp-init--[name]
directive("init", ({ directives: { init }, evaluate }) => {
init.forEach((entry) => {
useInit(() => evaluate(entry));
});
});
with this:
import { useEffect } from "preact/hooks";
// ...
// data-wp-init--[name]
directive("init", ({ directives: { init }, evaluate }) => {
init.forEach((entry) => {
useEffect(() => evaluate(entry), []);
});
});
Exporting our own useInit
, useWatch
, useEffect
, useMemo
… with integrated withScope
is fine, we should keep doing that, because people using them in data-wp-run
won't use them with evaluate
.
packages/e2e-tests/plugins/interactive-blocks/directive-on-document/render.php
Outdated
Show resolved
Hide resolved
packages/e2e-tests/plugins/interactive-blocks/directive-on-window/render.php
Outdated
Show resolved
Hide resolved
d855787
to
99e2ed8
Compare
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.
The code looks good to me 🙂
I have a few suggestions/questions for the docs.
Flaky tests detected in c8635c4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7570845563
|
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 now! Thanks, Carlos.
It seems like the e2e tests are failing though... |
I accidentaly removed one check, that waits for the counter to appear again. Fixed in f9eb31c |
await page.keyboard.press( 'ArrowDown' ); | ||
await expect( counter ).toHaveText( '1' ); | ||
} ); | ||
test( 'the event listener is removed when the element is removed', async ( { |
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.
@c4rl0sbr4v0, this test is flaky it failed 20+ times since PR got merged. Ref: #57983.
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.
What?
Add two new directives.
Why?
As commented in the respective discussion.
data-wp-on-window
anddata-wp-on-document
will be directives widely used to add event listeners.How?
Testing Instructions
E2E tests are included, but feel free to use the new directives!
Testing Instructions for Keyboard
Screenshots or screencast