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

Jump to bottom

Try restoring the site editor animation #51956

Merged
merged 9 commits into from
Jul 4, 2023
Merged

Try restoring the site editor animation #51956

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cf0be29
Try restoring the site editor animation
talldan Jun 27, 2023
af387c5
fix header animation
SaxonF Jun 27, 2023
9d8f62b
Remove accidental addition of layout prop
talldan Jun 27, 2023
e1274aa
tidy up formatting
talldan Jun 27, 2023
2425841
fix animate presence issue
SaxonF Jun 27, 2023
7fa2fdc
Merge branch 'try/restore-site-editor-animation' of https://github.co…
SaxonF Jun 27, 2023
3645aef
Fix animation between sidebar view and distraction free edit view
jeryj Jun 27, 2023
828f2da
Leave sidebar present and maintain canvas to
jeryj Jun 29, 2023
b91069e
Fix mobile view for pattern library
jeryj Jun 30, 2023
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
33 changes: 17 additions & 16 deletions packages/edit-site/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,11 +228,20 @@ export default function Layout() {
ariaLabel={ __( 'Editor top bar' ) }
as={ motion.div }
variants={ {
isDistractionFree: { opacity: 0 },
isDistractionFreeHovering: { opacity: 1 },
view: { opacity: 1 },
edit: { opacity: 1 },
isDistractionFree: { opacity: 0, y: 0 },
isDistractionFreeHovering: {
opacity: 1,
y: 0,
},
view: { opacity: 1, y: '-100%' },
edit: { opacity: 1, y: 0 },
} }
exit="view"
initial={
isDistractionFree
? 'isDistractionFree'
: 'view'
}
transition={ {
type: 'tween',
duration: disableMotion ? 0 : 0.2,
Expand All @@ -247,21 +256,13 @@ export default function Layout() {

<div className="edit-site-layout__content">
<AnimatePresence initial={ false }>
Copy link
Contributor

Choose a reason for hiding this comment

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

<AnimatePresence /> is only needed when we need to animate removing an element. We're not removing it from the DOM, just hiding it from user interactions, so we don't need to use this anymore.

{
{ showSidebar && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevin940726 mentioned to me that the issue with completely unmounting the sidebar is that it contains hooks that sync the routing state, and while it's unmounted those hooks aren't running, so some navigation doesn't work as expected. This is the reason it was changed in #51558.

@youknowriad Wondering if you have any suggestions for solving that in a different way?

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, here's a bug I found while testing on mobile:

Kapture.2023-06-27.at.14.54.22.mp4

The previous sidebar still exists but hidden if we use the browser's back button to go back from the edit mode. It has a higher z-index value too so it intercepts the click event (the add new pattern dropdown shouldn't be opened).

There might be a better and more elegant solution for this though! I didn't have time so I settled with the CSS hack 🤦 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Also related is the issue found here that actually existed before that PR and is potentially related to AnimatePresence not unmounting properly. The added complication is we now need the sidebar mounted on mobile for routing reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

it contains hooks that sync the routing state

Can you clarify which hooks we're talking about here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevin940726 Do you know the which specific code might be causing that issue?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's useSyncPathWithURL which is called inside the sidebar. I'm not entirely sure if we need it here though, this whole thing is a bit out of my league.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we move useSyncPathWithURL to the layout component instead?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, that hook relies on useNavigator which relies on the Navigator context, so we probably have to move the whole NavigatorProvider too. Last time I tried it failed on a weird error which I can't recall. To minimize the scope of the PR, fixing the CSS seems like an easier solution for now.

<motion.div
layout
initial={ {
opacity: 0,
} }
animate={
showSidebar
? { opacity: 1, display: 'block' }
: {
opacity: 0,
transitionEnd: {
display: 'none',
},
}
}
animate={ { opacity: 1 } }
exit={ {
opacity: 0,
} }
Expand All @@ -282,7 +283,7 @@ export default function Layout() {
<Sidebar />
</NavigableRegion>
</motion.div>
}
) }
</AnimatePresence>

<SavePanel />
Expand Down
Loading