Trigger sidebar animations only on cross-route navigations #61402
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
jsnajdr
added
[Type] Bug
|
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: +50 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
I'm unable to reproduce this issue on current |
|
I can repro now in |
|
I was also able to reproduce this problem using trunk. However, in this PR, an unnatural behavior can be seen when switching the sidebar content. The sidebar content appears to switch immediately without animation, followed by the animation. 59810206bbf469e68b65e92398ec139a.mp4 |
oandregal
reviewed
May 6, 2024
| // Navigation state that is updated when navigating back or forward. Helps us | ||
| // manage the animations and also focus. | ||
| function createNavState() { | ||
| let state = { |
There was a problem hiding this comment.
An alternative fix would be to set the initial direction to forward, which would be a one-line change in trunk. What would you think of this?
I can't think of any example where setting the initial direction would break: because we'd always change the routeKey for different screens the sidebar will always be animated. We just need direction for determining how to animate it (right-to-left or the other way around).
The closure created based on an initializator function for useState is a nice trick, though I find it less readable than the current code in trunk. Unless I'm missing an use case, from a code maintenance POV I'd favor just setting the initial direction.
There was a problem hiding this comment.
An alternative fix would be to set the initial direction to
forward, which would be a one-line change in trunk. What would you think of this?
That would trigger the forward animation also on initial load, and we don't want that. That's the reason why null, or "no animation" also needs to be one of the states. We set forward or back only on actual navigation, triggered by a click.
The closure created based on an initializator function for
useStateis a nice trick
I don't agree it's a trick, here we're simply creating a NavState object and storing it permanently in local state. It's a Gutenberg convention that we avoid "real" classes (new NavState()) and instead prefer create* functions that store the object private data in local variables (a closure) and return an interface object.
Maybe it's the useState without any setState that is confusing? An alternative way to write this is:
const navState = useMemo( createNavState, [] );This does the same thing, it only has the theoretical disadvantage that React is allowed to "forget" the memoized values when it feels it has too little memory or whatever.
There was a problem hiding this comment.
That would trigger the forward animation also on initial load
Ah, that's true. Thanks, I had missed that.
It's a Gutenberg convention that we avoid "real" classes (new NavState()) and instead prefer create* functions that store the object private data in local variables (a closure) and return an interface object.
I haven't come across this pattern until now. Of course, being such a big project it may be used somewhere and I just didn't come across it.
The essence of what we want to do is to only recompute the wrapperCls when the routeKey actually changes. What if we are explicit about that? We could do that by memoizing wrapperCls and making it dependant on routeKey, for example. Patch to be applied totrunk:
--- a/packages/edit-site/src/components/sidebar/index.js
+++ b/packages/edit-site/src/components/sidebar/index.js
@@ -12,6 +12,7 @@ import {
useState,
useRef,
useEffect,
+ useMemo,
} from '@wordpress/element';
import { focus } from '@wordpress/dom';
@@ -48,10 +49,13 @@ export default function SidebarContent( { routeKey, children } ) {
elementToFocus?.focus();
}, [ navState ] );
- const wrapperCls = clsx( 'edit-site-sidebar__screen-wrapper', {
- 'slide-from-left': navState.direction === 'back',
- 'slide-from-right': navState.direction === 'forward',
- } );
+ // We are only interested in recomputing wrapperCls when the routeKey changes.
+ const wrapperCls = useMemo( () => {
+ return clsx( 'edit-site-sidebar__screen-wrapper', {
+ 'slide-from-left': navState.direction === 'back',
+ 'slide-from-right': navState.direction === 'forward',
+ } );
+ }, [ routeKey ] );
return (
<SidebarNavigationContext.Provider value={ navigate }>Would that work for you? Is there scenario where this approach would break?
There was a problem hiding this comment.
I think I tried exactly this when working on this PR, and it works, but unfortunately it also triggers ESLint warnings:
React Hook useMemo has a missing dependency: 'navState.direction'.
React Hook useMemo has an unnecessary dependency: 'routeKey'.
The code I propose is basically an attempt to avoid these warnings:
- By moving the
wrapperClscalculation to a child component that getsrouteKeyaskey, I achieve the "recompute whenrouteKeychanges" behavior without ever usingrouteKeyin a hook dependency array. Because that's always going to be a problem becauserouteKeyis not used inside the hook. - By creating a constant
navStateobject and callingnavState.get()inside the hook, I avoid having to add the actualnavState.directionvalue as a hook dependency. I only add the constantnavStateone.
So, I see your point, maybe I overcomplicated things just by trying to avoid eslint-disable. But I'm not sure if there is a better solution that satisfies all the constraints.
There was a problem hiding this comment.
I'm going to merge the PR now as is. If we arrive at some actionable suggestion as how to improve the code (navState, routeKey) I'm happy to do it as a followup.
Thanks @t-hamano for spotting this little bug. I was able to fix it (791211f) by using |
t-hamano
approved these changes
May 7, 2024
There was a problem hiding this comment.
LGTM!
The discussion in this comment is a bit difficult for me, so I don't have an opinion on how to implement it, but everything is working as expected so I'd like to approve.
jsnajdr
force-pushed
the
fix/sidebar-animations
branch
from
791211f
to
7c861df
Compare
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.
In Site Editor, when you're on the
path=/pageview, in the sidebar you can click on "types" of pages. And there is a bug that after initial load, clicking on the "Drafts" item, for example, triggers a "slide from right" animation that makes it look like you are navigating to another nested sidebar:Screen.Recording.2024-05-03.at.14.33.18.mov
But that animation shouldn't be there, we want to trigger animations only when the displayed sidebar really changes.
This PR fixes that by making sure that the animations and focusing happen only when a new sidebar component is mounted, i.e., when the
key={ routeKey }prop of theSidebarContentWrappercomponent changes.It's regression introduced in #60466.