Site Editor: Only show 'Back' button when user came from an editor canvas#58710
Merged
noisysocks merged 5 commits intotrunkfrom Feb 7, 2024
Merged
Conversation
|
Size Change: -222 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Feb 6, 2024
| previousLocation?.params.canvas === 'edit'; | ||
| const showBackButton = isFocusMode && didComeFromEditorCanvas; | ||
| return showBackButton ? () => history.back() : undefined; | ||
| }, [ location ] ); |
Contributor
There was a problem hiding this comment.
This PR's testing nicely for me! Just noticed that the linter is complaining about missing items in the dependency array for this one:
Is it okay to include those, too?
Member
Author
There was a problem hiding this comment.
Hm. When I add previousLocation everything stops working as it seems to update too often. Maybe previousLocation isn't a stable reference....
How much of a "rule" is this lint rule? 😀
Contributor
There was a problem hiding this comment.
I see lots of // eslint-disable-next-line react-hooks/exhaustive-deps in the code base. I think it's one of those rules that we should attempt to honour when we can, but if we can't, then it's good to add in that disable line with a line or two with a Ignore reason or Disable reason with a quick explanation 🙂
Member
Author
There was a problem hiding this comment.
OK I figured it out. usePrevious had a flawed implementation in that it returns what value was the previous time that the component rendered. This is different to what we want, and what I think most consumers want, which is that usePrevious should return what value previously was.
If the component updates for some reason unrelated to value changing (e.g. its state changed) then we'll get the current value instead of the previous value.
I found this article which explains the problem and solution really well:
https://www.developerway.com/posts/implementing-advanced-use-previous-hook
I also saw that the implementation that we based our version of usePrevious on has been changed to be similar to the approach suggested in the article above:
https://github.com/uidotdev/usehooks/blob/main/index.js#L1017
So, I updated our implementation of usePrevious to follow. I also added a unit test since there was none. Now, when I set deps to be exhaustive, the back button updates properly.
Contributor
There was a problem hiding this comment.
Great catch — I think this is probably the kind of flaw that the folks who introduced that linting rule were hoping we'd uncover!
It looks like the unit tests are failing now, in particular some ToolsPanel tests, which use usePrevious internally. If that winds up being complex to solve, since usePrevious / the compose package is a bit lower level than the proposed change in this PR, would it be worth splitting out the idea into two parts — 1) add in the eslint-disable-next-line for this PR to get things working for now, and 2) open a separate PR for usePrevious where we can debug the ToolsPanel test, and see if there were components that expected the previous implementation?
Member
Author
There was a problem hiding this comment.
Yeah, bugger, let's do that.
Contributor
There was a problem hiding this comment.
- open a separate PR for
usePreviouswhere we can debug theToolsPaneltest, and see if there were components that expected the previous implementation?
I looked for it and take it this never materialized. I’ve long been wondering if usePrevious was going to need to be replaced across the codebase due to the fact that its implementation goes against the guidance to not read refs in render. I suppose that’s not been a problem as of yet. Still, it would seem to be another reason to replace it. I guess we’d have to keep the current one around for BC policy though.
|
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 Core SVNCore Committers: Use this line as a base for the props when committing in SVN: GitHub Merge commitsIf 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. |
youknowriad
reviewed
Feb 6, 2024
packages/edit-site/src/components/block-editor/use-site-editor-settings.js
Show resolved
Hide resolved
…of value from previous render See https://www.developerway.com/posts/implementing-advanced-use-previous-hook for a good explanation of the problem.
…instead of value from previous render" This reverts commit 8068fe3.
andrewserong
approved these changes
Feb 7, 2024
Contributor
andrewserong
left a comment
There was a problem hiding this comment.
Works nicely and the code change LGTM! ✨
Thanks for the back and forth!
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
5 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?
Follow up to #58528. In particular, addresses this comment: #58528 (comment).
In
trunkwe show the "Back" button in the site editor whenever you're in "focus mode" e.g. when looking at a template part, pattern, or editing a page's template. The back button appears even when you navigate to the current screen via the W menu.This PR changes the behaviour so that the "Back" button only appears when you navigate to to the current screen via the site editor canvas e.g. by clicking "Edit" in the toolbar of a template part.
Why?
We don't want to overuse the "Back" element. If a user navigates to a screen via the W menu then they can navigate back by clicking on W again.
How?
Track the previous location using
usePreviousand only show the back button if the previous location is an editor canvas (canvas=edit).Testing Instructions
Screenshots or screencast
Before:
before.mp4
After:
after.mp4