Fix browser history when synchronising state with urls #48731
Merged
Conversation
This file contains 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
youknowriad
added
[Type] Bug
|
Seems to fix the issue on my end! Nice one @youknowriad! Out of curiosity, on a separate note (not caused by this PR), when I click "Template Parts" and click "Footer", for example, I need to click the browser's back button twice to actually go back. Is this related or a separate issue? |
|
Size Change: +710 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Mar 3, 2023
|
@Mamaduka I can't reproduce this myself. Do you have some steps I could follow to see if I can reproduce? |
|
@Mamaduka which browser are you using? |
I can reproduce this issue, it seems there's still an extra history.push. Let's see if I can find it. |
|
My bad! It looks like this was a browser cache issue. Sorry for the confusion. P.S. I'm testing with Chrome. |
|
It turns out that when you do |
|
While the previous fix works, I'm also exploring if I can change how "push" works instead. |
|
85f2370 fixes the double back button issue but also appears to flash the "Home" submenu briefly. |
Mamaduka
reviewed
Mar 3, 2023
| @@ -32,7 +32,6 @@ export default function TemplateDetails( { template, onClose } ) { | |||
|
|
|||
| // TODO: We should update this to filter by template part's areas as well. | |||
| const browseAllLinkProps = useLink( { | |||
| canvas: 'view', | |||
There was a problem hiding this comment.
I think this causes regression of #48301.
There was a problem hiding this comment.
it should be "undefined" now because "view" is the default value. But I guess it suffers from the issue I talk about here #48731 (comment)
|
@youknowriad, I also noticed that if I refresh |
|
Ok, I think I've fixed all the issues. I'm not entirely satisfied with these syncing hooks and I believe we need e2e tests for all these flows but this should be good maybe for now? |
|
This is working perfectly when going straight from the dashboard to the site editor then hitting the back button. One thing I noticed is when you go dashboard > site editor > [back to templates sidebar view] then try the browser back button, there's a couple of extra dead back button clicks in there between the site editor and the dashboard. I tried to capture it in this gif:
|
|
@apeatling Noticing that too when I do the following:
I'm also still noticing the flash of the "Home" sidebar view when I do the following:
|
|
I think I fixed both of these issues. I'd appreciate some checks. @apeatling I also noticed that on your video you land directly on a sub menu when opening the site editor. I'm aware of this issue but it's specific to old WP versions (there's a redirect that we can't override in Gutenberg). If you test with WP trunk, you'll notice that you land on the "root" of the site editor. |
ntsekouras
reviewed
Mar 6, 2023
packages/edit-site/src/components/sync-state-with-url/use-sync-canvas-mode-with-url.js
Outdated
Show resolved
Hide resolved
|
I added some e2e tests to cover these flows. |
ntsekouras
reviewed
Mar 6, 2023
kevin940726
reviewed
Mar 6, 2023
ntsekouras
approved these changes
Mar 6, 2023
packages/edit-site/src/components/sidebar-navigation-screen-templates/index.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in ee28018. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4341886921
|
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: e064813 |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
ntsekouras
reviewed
Oct 31, 2023
packages/edit-site/src/components/sync-state-with-url/use-sync-canvas-mode-with-url.js
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
closes #48726
What?
This PRs fixes the browser history when navigating the site editor.
How?
Testing Instructions
1- Open the site editor
2- Click browser "back" button
3- you should be back in the dashboard properly
4- Testing using Chrome, Firefox or Edge (in trunk it's broken with these)