Interactivity API: Override unfinished navigate calls#54201
Merged
Conversation
DAreRodz
added
[Type] Enhancement
|
Size Change: +207 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 43556a0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6089995120
|
DAreRodz
requested review from
Mamaduka,
ajitbohra,
kevin940726,
nerrad and
ntwb
as code owners
21 tasks
luisherranz
requested changes
Sep 6, 2023
Member
luisherranz
left a comment
luisherranz
left a comment
There was a problem hiding this comment.
Very good test!! It's pretty smart 🙂
My only concern is why should we use url instead of href? (href also contains the hash)
packages/interactivity/src/router.js
Outdated
| // Navigate to a new page. | ||
| export const navigate = async ( href, options = {} ) => { | ||
| const url = cleanUrl( href ); | ||
| navigatingTo = url; |
Member
There was a problem hiding this comment.
What's the logic for using url instead of href here?
Contributor
Author
There was a problem hiding this comment.
That's a good question. 😄 I think I assumed that, as the url would be the same for href with hashes, both navigations would wait for the same Promise so that they would happen in order and there would be no problem.
I missed considering that:
- You could use
forceand request the HTML again. - Both navigations would trigger a render, which is bad.
Member
There was a problem hiding this comment.
Yeah, I think it's a bit safer this way. That way, the user will end up with the correct hash in the browser URL (even though we don't support scrolling to the id yet).
Thanks David!
luisherranz
approved these changes
Sep 6, 2023
Member
luisherranz
left a comment
luisherranz
left a comment
There was a problem hiding this comment.
Looks great, thanks David! 🙂
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Improve
navigate()to render only the result of the last call when multiple happen simultaneously.Why?
Required for the Query Loop block with enhanced pagination.
store()API #53740How?
Previous
navigate()calls are not canceled, so the related HTML is still fetched. However, only the last one executes Preact'srender().Testing Instructions