Try restoring the site editor animation #51956
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
talldan
added
[Type] Regression
talldan
requested review from
kevin940726,
SaxonF,
glendaviesnz and
aaronrobertshaw
|
Size Change: -36.4 kB (-3%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
92 tasks
talldan
commented
Jun 27, 2023
| @@ -247,21 +256,13 @@ export default function Layout() { | |||
|
|
|||
| <div className="edit-site-layout__content"> | |||
| <AnimatePresence initial={ false }> | |||
| { | |||
| { showSidebar && ( | |||
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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 🤦 .
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
it contains hooks that sync the routing state
Can you clarify which hooks we're talking about here?
There was a problem hiding this comment.
@kevin940726 Do you know the which specific code might be causing that issue?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can't we move useSyncPathWithURL to the layout component instead?
There was a problem hiding this comment.
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.
|
Flaky tests detected in 828f2da. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5415454086
|
…m/WordPress/gutenberg into try/restore-site-editor-animation
|
GIFs don't do this justice, so here'a s video showing the smooth animation restored: Screeny.Video.27.Jun.2023.at.11.58.07.movThis is so important to have in place, so thanks for working on this! |
What did exactly distraction free break? @jeryj and I worked a lot on respecting the animation while also adding the hover ones for DFM. It's important to know what we broke in the process :) |
|
I tested this and it does fix the animation for the frame and the editor header. However it introduces a weird entry animation when in Distraction Free Mode: Trunkdfm-entry-animation-trunk.mp4This PRdfm-entry-animation-51956.mp4This PR makes the header in DFM do a weird fade out and the site hub looks out of place with nothing to support it behind. @jeryj you may have some ideas? |
jeryj
reviewed
Jun 27, 2023
jeryj
reviewed
Jun 27, 2023
@draganescu All I know is that when the regression that was accidentally introduced by the Library was reverted, there was still a remaining issue with the header animation that seemed to be last modified by the distraction free PR. Worth noting that the library change was merged before distraction free, so it's possible that it masked the problems. |
|
Status update - I believe this PR still has the bug being discussed here. I'm not familiar with the routing code. I'll be going AFK for a while soon and won't have time to look into it, so any help appreciated on this. Please feel free to push to this PR or open up an alternative PR using any code from this PR. It might be worth adding an end-to-end test case for the bug to demonstrate the issue (cc @kevin940726). I think there are really two options for moving forwards:
|
I can try working on this. |
|
When the sidebar exits, should it:
The current behavior on this branch is for it to also slide to the left, but I wasn't sure if that was intentional. This screen recording shows both animations. First two clicks are the slide out animation and the second two are only an opacity fadein/out. Sidebar.animation.mov |
|
Only fade, the slide out was a bug previously as well. |
sidebar animation The sidebar is necessary for routing on mobile so we have to maintain its presence in the DOM. Just hiding it isn't enough though, as it is still able to be reached with keyboard tabs and screen readers. Using the relatively new `inert` property disables the element from user interaction, so we add that when we don't want the sidebar to be shown.
jeryj
reviewed
Jun 29, 2023
| </NavigableRegion> | ||
| ) } | ||
| </AnimatePresence> | ||
| </motion.div> | ||
|
|
||
| <div className="edit-site-layout__content"> | ||
| <AnimatePresence initial={ false }> |
There was a problem hiding this comment.
<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.
|
This is ready for review, if anyone wants to hop in :) |
|
This is what I'm getting on mobile viewport after clicking on a pattern category 😅
|
That... could be better 😅 I'll take a look! |
On Mobile, the canvas mode wasn't being set to edit when using the pattern library. This updates it to use the showSidbar value instead, keeping it in sync with the inert setting.
|
@kevin940726 Fixed! Thanks for catching that. |
|
@jeryj is this PR something that could also address this #51173 (comment) ? It seems the site hub is rendered as [Object object]? |
|
I don't think this is blocking:
|
draganescu
approved these changes
Jul 3, 2023
There was a problem hiding this comment.
I tested this and it works very well for what I tried. I'll leave the final call to @kevin940726 because I am not sure I am looking at all the things in the mobile case.
kevin940726
approved these changes
Jul 4, 2023
There was a problem hiding this comment.
Tested it and it works! Thanks for picking this up, @jeryj !
|
Should this be cherry-picked to 6.3? The issue it fixes is in the 6.3 board. |
tellthemachines
added
the
Backport to WP 6.7 Beta/RC
|
I just cherry-picked this PR to the update/second-round-RC1 branch to get it included in the next release: d733cc5 |
tellthemachines
pushed a commit
that referenced
this pull request
Jul 17, 2023
* Try restoring the site editor animation * fix header animation * Remove accidental addition of layout prop * tidy up formatting * fix animate presence issue * Fix animation between sidebar view and distraction free edit view * Leave sidebar present and maintain canvas to sidebar animation The sidebar is necessary for routing on mobile so we have to maintain its presence in the DOM. Just hiding it isn't enough though, as it is still able to be reached with keyboard tabs and screen readers. Using the relatively new `inert` property disables the element from user interaction, so we add that when we don't want the sidebar to be shown. * Fix mobile view for pattern library On Mobile, the canvas mode wasn't being set to edit when using the pattern library. This updates it to use the showSidbar value instead, keeping it in sync with the inert setting. --------- Co-authored-by: Saxon Fletcher <[email protected]> Co-authored-by: Jerry Jones <[email protected]>
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
ramonjd
added a commit
that referenced
this pull request
Jul 18, 2023
* Try restoring the site editor animation (#51956) * Try restoring the site editor animation * fix header animation * Remove accidental addition of layout prop * tidy up formatting * fix animate presence issue * Fix animation between sidebar view and distraction free edit view * Leave sidebar present and maintain canvas to sidebar animation The sidebar is necessary for routing on mobile so we have to maintain its presence in the DOM. Just hiding it isn't enough though, as it is still able to be reached with keyboard tabs and screen readers. Using the relatively new `inert` property disables the element from user interaction, so we add that when we don't want the sidebar to be shown. * Fix mobile view for pattern library On Mobile, the canvas mode wasn't being set to edit when using the pattern library. This updates it to use the showSidbar value instead, keeping it in sync with the inert setting. --------- Co-authored-by: Saxon Fletcher <[email protected]> Co-authored-by: Jerry Jones <[email protected]> * Change password input to type text so contents are visible. (#52622) * Iframe: Silence style compat warnings when in a BlockPreview (#52627) * Do not autofocus page title field in the Create new page modal dialog. (#52603) * Use lowercase p in "Manage Patterns" (#52617) * Remove theme patterns title (#52570) * Block editor store: also attach private APIs to old store descriptor (#52088) As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr * Block removal prompt: let consumers pass their own rules (#51841) * Block removal prompt: let consumers pass their own rules Following up on #51145, this untangles `edit-site` from `block-editor` by removing the hard-coded set of rules `blockTypePromptMessages` from the generic `BlockRemovalWarningModal` component. Rules are now to be passed to that component by whichever block editor is using it. Names and comments have been updated accordingly and improved. * Site editor: Add e2e test for block removal prompt * Fix Shift+Tab to Block Toolbar (#52613) * Fix Shift+Tab to Block Toolbar * Add changelog entry * Show warning on removal of Post Template block in the site editor. (#52666) * Avoid copying global style presets via the styles compatibility hook (#52640) * i18n: Make the tab labels of `ColorGradientSettingsDropdown` component translatable (#52669) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save (#52682) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save * Address feedback * Patterns: Remove `reusable` text from menu once rename hint has been dismissed (#52664) * Show uncategorized patterns on the Editor > Patterns page (#52633) * Patterns: fix bug with Create Patterns menu not showing in site editor page editing (#52671) * Pass the root client id into the reusable blocks menu * Check that clientIds array is defined * Make check for array item more specific * Search block: Enqueue view script through block.json (#52552) * Search block: Enqueue view script through block.json * Remove extra space * Use `_get_block_template_file` function and set $area variable. (#52708) * Use `_get_block_template_file` function and set $area variable. * Update packages/block-library/src/template-part/index.php Co-authored-by: Felix Arntz <[email protected]> --------- Co-authored-by: Felix Arntz <[email protected]> * Site Editor: Don't allow creating template part on the Patterns page for non-block themes (#52656) * Don't allow template part to be created on the Patterns page for non-block themes * Remove unnecessary theme directory name in E2E test * Change Delete page menu item to Move to trash. (#52641) * Use relative path internally to include packages in dependencies (#52712) * Spacing Sizes: Fix zero size (#52711) * DimensionsPanel: Fix unexpected value decoding/encoding (#52661) --------- Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Saxon Fletcher <[email protected]> Co-authored-by: Jerry Jones <[email protected]> Co-authored-by: Robert Anderson <[email protected]> Co-authored-by: Andrea Fercia <[email protected]> Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: James Koster <[email protected]> Co-authored-by: Miguel Fonseca <[email protected]> Co-authored-by: Haz <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Ella <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Carolina Nymark <[email protected]> Co-authored-by: Petter Walbø Johnsgård <[email protected]> Co-authored-by: Jonny Harris <[email protected]> Co-authored-by: Felix Arntz <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: Andrew Serong <[email protected]>
This was referenced Jul 25, 2023
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.
What?
Attempts to fix #51903
How?
Restores how the code looked before (relying onAnimatePresenceto handle entry and exit animations rather than thedisplayproperty). Unfortunately bringing this back reintroduces routing issues on mobile that were solved by #51558.Since we're not sure how to fix the routing issues on mobile, we're leaving the sidebar present ini the DOM, but hiding it visually and marking it as
inertto stop all user interactions on the sidebar.Also resolves some issues with the editor header animation that seem to have been introduced by the addition of Distraction Free mode.
Testing Instructions
Screenshots or screencast
Before
Kapture.2023-06-27.at.13.19.00.mp4
After
Kapture.2023-06-27.at.13.20.49.mp4