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.

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

Navigator: disable initial animation #49062

Merged
merged 6 commits into from
Mar 15, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Mar 14, 2023

What?

Alternative approach to #48630
Partially addresses #48595

Change the Navigator component so that the very fist NavigatorScreen doesn't perform its initial animation when rendered on screen.

Why?

As described in #48595 , we'd like to disable such animation the component is first rendered, while maintaining the animation when navigating across different menus.

How?

Added a check that will disable the animation if the NavigatorScreen being shown is the initial screen (and it's not a backwards navigation).

Testing Instructions

Storybook:

  • Load the Storybook examples
  • Make sure that the component doesn't animate on first render
  • Make sure that the component animates on every following render (both forwards and backwards)

Site editor:

  • Open the site editor
  • Make sure that the menu in the left sidebar doesn't animate on first render
  • Navigate though the left sidebar menus, make sure that the menus animate in
  • Edit a template by clicking the pencil icon (the left sidebar should close)
  • Click the "W" logo to reopen the sidebar. Make sure that the menu in the sidebar doesn't animate on its first render
  • Navigate to the parent menu, and make sure that the animation is rendered

Check for regressions in other areas of the editor where Navigator is used (the initial animation should be disabled there too, but everything else should work as expected):

  • Global styles sidebar
  • Preferences modal (on small viewports)

Screenshots or screencast

(Note: in the videos I slowed the Navigator animation to be 2 seconds long, to really showcase the changes from this PR)

Before:

Navigator.no.initial.animation.-.BEFORE.mp4

After:

Navigator.no.initial.animation.-.AFTER.mp4

@ciampo ciampo self-assigned this Mar 14, 2023
@ciampo ciampo added the [Package] Components /packages/components label Mar 14, 2023
@ciampo
Copy link
Contributor Author

ciampo commented Mar 14, 2023

Unrelated to this PR:

while testing for this fix, I realised that there is a flash on unstyled text ("Dashboard") when navigating from the "Templates" menu to the "Design" menu.

Screenshot 2023-03-14 at 14 06 19

@jasmussen
Copy link
Contributor

I don't think that's unstyled text, I think that's the tooltip that appears. But I would agree, it would be nice if that tooltip was delayed by something like 2s.

Copy link
Member

@tomdevisser tomdevisser left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @ciampo! I think it's almost there with this change, except for the opacity change when switching between editor/sidebar. The idea from the design viewpoint was that the text is already there, so it shouldn't animate or fade in.

@jasmussen
Copy link
Contributor

Looks good!

Overall it's good to think of these orchestrations not as disparate animations, but as materials that behave according to each other. In this case you could imagine the drilldown navigation to be one long horizontal scroll, that as you literally drilldown moves leftwards, and as you drill back up moves rightwards. In that light, moving it leftwards when you go into editing makes sense, because the canvas pushes it away, and it moves rightwards when you exist the fullscreen editor, because it's pulled back in.

The fades are a separate aspect. I could do with or without them, as they aren't related to the material.

@jameskoster
Copy link
Contributor

In that light, moving it leftwards when you go into editing makes sense, because the canvas pushes it away, and it moves rightwards when you exist the fullscreen editor, because it's pulled back in.

Haha, this behaviour is actually that #48595 was seeking to eliminate :) The motivator being the idea that the Frame (as suggested by its shadow) exists on a layer above the dark material. So entering full-screen edit mode doesn't push the menu away, instead the menus remain in place and the Frame simply expands to cover it visually.

To clarify in slowmo, the aim was this:

slowmo.mp4

Rather than:

slowmo.mp4

@ciampo
Copy link
Contributor Author

ciampo commented Mar 14, 2023

Thank you all for the feedback!

I think that we should differentiate between the two main sources of "animation" that, combined, contribute to the scenario that we're discussing:

  • The Navigator component, which is responsible for animating the different "menu screens" when interacting with each menu item
  • The sidebar container (I believe called "Site hub" in code), responsible for the open/close/resize animation of the whole sidebar

My suggestion is that we keep this PR focused on the Navigator component, since any changes to the component can have implications in other parts of the editor too (for example, Global Styles and the preferences modal on small viewports). This PR can basically be the first step towards #48595 .

The way the menu (ie. Navigator) gets resized/translated/cropped can be handled in a separate PR, since it's specific to the "Site Hub" container and the @wordpress/edit-site package.

@ciampo ciampo added the [Type] Enhancement A suggestion for improvement. label Mar 14, 2023
@ciampo ciampo marked this pull request as ready for review March 14, 2023 15:28
@ciampo ciampo requested a review from ajitbohra as a code owner March 14, 2023 15:28
@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Flaky tests detected in f51d02a.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4426378795
📝 Reported issues:

@jasmussen
Copy link
Contributor

Haha, this behaviour is actually that #48595 was seeking to eliminate :) The motivator being the idea that the Frame (as suggested by its shadow) exists on a layer above the dark material. So entering full-screen edit mode doesn't push the menu away, instead the menus remain in place and the Frame simply expands to cover it visually.

Ah yes, thanks for the reminder and apologies for the detour!

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Code looks good and tests well 👍

@ciampo ciampo force-pushed the feat/navigator-disable-initial-animation branch from 5815a57 to f51d02a Compare March 15, 2023 12:39
@ciampo ciampo enabled auto-merge (squash) March 15, 2023 12:40
@ciampo ciampo merged commit 352b836 into trunk Mar 15, 2023
@ciampo ciampo deleted the feat/navigator-disable-initial-animation branch March 15, 2023 13:18
@github-actions github-actions bot added this to the Gutenberg 15.4 milestone Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants