-
Notifications
You must be signed in to change notification settings - Fork 4.4k
iAPI Router: Add support for new router regions with attachTo
#70421
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
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +150 B (+0.01%) Total Size: 1.86 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.
Could you add an explanation to the documentation of the README.md
file? I would also clarify what happens in each case:
- the region exists on both pages
- the region exists on the current page but not on the new page
- the region exists on the new page but not on the current page.
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.
Regarding the bug itself, I can confirm it solves the original issue combined with the mentioned PR:
Before
Regions.-.not.working.mp4
After
Regions.-.working.mp4
I believe it is not related to this pull request. This is something I noticed as well and it happens in The problem is that we are hiding the original image with this directive, so if the lightbox overlay is not loaded yet, there is a moment when there is no image at all. Context for this change can be found in this pull request. I am not sure what is the best approach there, to be honest. |
Then there's no problem, thank you! |
8449c0e
to
135a016
Compare
README.md updated, @luisherranz. |
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, thanks David!
What?
Warning
This PR has been created on top of #70353. Wait for that PR to be merged before merging this one.
Renders missing router regions from newly visited pages when they have an
attachTo
property defined.The
attachTo
property should be a valid CSS selector, pointing to the parent element where the new router region should be rendered.E.g., this region would be rendered in
<body>
if it appears in a visited page, even when it wasn't initially rendered.Why?
To support regions that don't exist in the previous page, like for example the overlay of the image lightbox.
How?
Inside
regionsToVdom()
, when a new page is processed, aregionsToAttach
object is created, mapping the ID of each router region withattachTo
to the CSS selector of the target parent element. The vDOM tree of these regions is still stored inregions
as usual.Later, inside
renderRegions()
, when that page is rendered, all regions in the current HTML are updated with the new content. Regions withattachTo
could already be present in the current HTML, and these are updated as previously. Only new regions withattachTo
are handled differently, creating a new root fragment for each of them in the corresponding target defined byattachTo
.If any of these regions disappear on subsequent pages, they are unmounted as expected.
Testing Instructions
There are e2e tests for this feature.
Also, this feature can be tested manually. For that, you have to ensure the changes in #70416 are included, and follow these steps:
/?query-1-page=999
). You should see something like "Sorry, but nothing was found. Please try a search with different keywords".