-
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
Interactivity API: add Slot and Fill directives #53958
Interactivity API: add Slot and Fill directives #53958
Conversation
Size Change: +268 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
I'm going to leave the new directives' documentation for a subsequent PR. @SantosGuillamot, I'd appreciate a quick review here, as this is blocking #53737. 🙏 Thanks! |
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've been testing it, and it seems to be working fine 👏 Some questions/concerns I have:
- Regarding the docs, let's not delay the subsequent PR in case anyone wants to start testing it.
- It seems I cannot fill multiple slots with the same fill. Is this expected? Something like this:
<span
data-wp-fill="state.slot1"
data-wp-fill="state.slot2"
>
Fill
</span>
- It seems I cannot fill the same slot with multiple fill. Is this expected? Something like this:
<span data-wp-fill="state.slot">Fill 1</span>
<span data-wp-fill="state.slot">Fill 2</span>
- Related to the previous point, did you consider defining the position in the fill instead of the slot? I didn't think too much about it, asking because you may have more context.
- It seems that there were some open comments in the original PR: link. Did we address them? If so, it'd could be nice to drop a comment there.
Haha, a lot of questions, as usual, @SantosGuillamot. 😝 Let's go one by one.
Roger that. 👍
It currently is; it's subject to change in the future though, while we polish the Slot and Fill API. 🙂
Same answer as before.
I'm not sure I have understood. Do you mean specifying a selector of some kind in the fill directive so you don't have to point to a slot? E.g., <div id="some-id"></div>
<p data-wp-fill='{ "anchor": "#some-id", "position": "after" }'>I'll be placed inside #some-id</p> Maybe this could have been the API if we had used Preact portals instead of Preact slots. 🤔 Well, I decided to test the Slot and Fill pattern for the Comments Form, as the whole Comments block is turned into a Preact application―already containing the Comments Form block―so it was more reasonable (for me) to use that pattern (portals would have had more sense if we were rendering Preact outside of a Preact app). In any case, I guess this could be a good API for the
Indeed, this is something I wanted to change. Surprisingly, the implementation stopped working as expected if I changed the priority order, so I decided to leave the current values for now. I'll post a comment in the original PR. 👍 |
This should be done in a separate PR.
456c456
to
cb8eacb
Compare
What?
Adds three new directives that implement the Slot and Fill pattern:
data-wp-slot-provider
data-wp-slot
data-wp-fill
It's a basic implementation that will be improved in the future.
Currently, it only supports a single fill inside a given slot. It also keeps the fill in its original position if there's not a matching slot.
Why?
Required by #53737.
How?
Defining a set of preact components that follows the same logic as the developit/preact-slots library but with preact signals, which greatly simplifies the code.
Those components are used inside of the aforementioned directives.
Testing Instructions
E2E tests should pass.