Skip to content
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.

Sign up for GitHub

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

Trigger sidebar animations only on cross-route navigations #61402

Merged
merged 2 commits into from
May 9, 2024
Merged

Trigger sidebar animations only on cross-route navigations #61402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06acccc
Trigger sidebar animations only on cross-route navigations
jsnajdr May 6, 2024
7c861df
Use layout effect when triggering animation
jsnajdr May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/sidebar-navigation-item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export default function SidebarNavigationItem( {
...props
} ) {
const history = useHistory();
const navigate = useContext( SidebarNavigationContext );
const { navigate } = useContext( SidebarNavigationContext );

// If there is no custom click handler, create one that navigates to `path`.
function handleClick( e ) {
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/sidebar-navigation-screen/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export default function SidebarNavigationScreen( {
);
const location = useLocation();
const history = useHistory();
const navigate = useContext( SidebarNavigationContext );
const { navigate } = useContext( SidebarNavigationContext );
const backPath = backPathProp ?? location.state?.backPath;
const icon = isRTL() ? chevronRight : chevronLeft;

Expand Down
94 changes: 59 additions & 35 deletions packages/edit-site/src/components/sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,62 +7,86 @@ import clsx from 'clsx';
* WordPress dependencies
*/
import {
useCallback,
createContext,
useContext,
useState,
useRef,
useEffect,
useLayoutEffect,
} from '@wordpress/element';
import { focus } from '@wordpress/dom';

export const SidebarNavigationContext = createContext( () => {} );
// Focus a sidebar element after a navigation. The element to focus is either
// specified by `focusSelector` (when navigating back) or it is the first
// tabbable element (usually the "Back" button).
function focusSidebarElement( el, direction, focusSelector ) {
let elementToFocus;
if ( direction === 'back' && focusSelector ) {
elementToFocus = el.querySelector( focusSelector );
}
if ( direction !== null && ! elementToFocus ) {
const [ firstTabbable ] = focus.tabbable.find( el );
elementToFocus = firstTabbable ?? el;
}
elementToFocus?.focus();
}

export default function SidebarContent( { routeKey, children } ) {
const [ navState, setNavState ] = useState( {
// Navigation state that is updated when navigating back or forward. Helps us
// manage the animations and also focus.
function createNavState() {
let state = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 useState is 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. By moving the wrapperCls calculation to a child component that gets routeKey as key, I achieve the "recompute when routeKey changes" behavior without ever using routeKey in a hook dependency array. Because that's always going to be a problem because routeKey is not used inside the hook.
  2. By creating a constant navState object and calling navState.get() inside the hook, I avoid having to add the actual navState.direction value as a hook dependency. I only add the constant navState one.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

direction: null,
focusSelector: null,
} );
};

const navigate = useCallback( ( direction, focusSelector = null ) => {
setNavState( ( prevState ) => ( {
direction,
focusSelector:
direction === 'forward' && focusSelector
? focusSelector
: prevState.focusSelector,
} ) );
}, [] );
return {
get() {
return state;
},
navigate( direction, focusSelector = null ) {
state = {
direction,
focusSelector:
direction === 'forward' && focusSelector
? focusSelector
: state.focusSelector,
};
},
};
}

function SidebarContentWrapper( { children } ) {
const navState = useContext( SidebarNavigationContext );
const wrapperRef = useRef();
useEffect( () => {
let elementToFocus;
if ( navState.direction === 'back' && navState.focusSelector ) {
elementToFocus = wrapperRef.current.querySelector(
navState.focusSelector
);
}
if ( navState.direction !== null && ! elementToFocus ) {
const [ firstTabbable ] = focus.tabbable.find( wrapperRef.current );
elementToFocus = firstTabbable ?? wrapperRef.current;
}
elementToFocus?.focus();
const [ navAnimation, setNavAnimation ] = useState( null );

useLayoutEffect( () => {
const { direction, focusSelector } = navState.get();
focusSidebarElement( wrapperRef.current, direction, focusSelector );
setNavAnimation( direction );
}, [ navState ] );

const wrapperCls = clsx( 'edit-site-sidebar__screen-wrapper', {
'slide-from-left': navState.direction === 'back',
'slide-from-right': navState.direction === 'forward',
'slide-from-left': navAnimation === 'back',
'slide-from-right': navAnimation === 'forward',
} );

return (
<SidebarNavigationContext.Provider value={ navigate }>
<div ref={ wrapperRef } className={ wrapperCls }>
{ children }
</div>
);
}

export default function SidebarContent( { routeKey, children } ) {
const [ navState ] = useState( createNavState );

return (
<SidebarNavigationContext.Provider value={ navState }>
<div className="edit-site-sidebar__content">
<div
ref={ wrapperRef }
key={ routeKey }
className={ wrapperCls }
>
<SidebarContentWrapper key={ routeKey }>
{ children }
</div>
</SidebarContentWrapper>
</div>
</SidebarNavigationContext.Provider>
);
Expand Down
Loading