Add rough animation to navigation and links #46342

artemiomorales · 2022-12-07T02:29:10Z

What?

This pull request is an exploration of how we might add animation to the navigation block and related links in the off canvas editor, related to #45884

Why?

The slide animations can help with the orient the user and overall provide a better experience.

How?

Currently, this PR adds a very rough implementation of the animation using Framer Motion so we can have something to look at as we continue discussions.

Testing Instructions

Open the Full Site Editor
Open the Navigation block
In the off-canvas editor, click the edit button on one of the navigation links
Click the back button on the navigation link's Block Card

Testing Instructions for Keyboard

I believe the same as above, except tab over to the buttons. Since this is just for a user test though, I can look at accessibility more closely once we have a better handle on the approach we're going to take.

Additional Notes and Question for Discussion

Note: The code currently activates the animation whenever the previously selected block is a navigation block, navigation link, or navigation submenu, so it's not a real implementation yet, so the code is very rough, though I'm happy to take any suggestions.

That being said, I'm looking for feedback on the animation itself — is this how we want it to look, or do we want to show an exit animation on the previously selected block before showing the entry animation of the newly selected one?

Screen shots or screencast

Screen.Recording.2022-12-06.at.9.02.07.PM.mov

draganescu · 2022-12-07T10:16:11Z

From the video it looks like a very good start. It's a bit jarring that we have a blank intermission, that is the present screen disappears too quickly and the future one comes from too far away. To guide in terms of animation tweaking use the global styles animation as a reference:

gs-animation.mp4

scruffian · 2022-12-07T12:23:42Z

The video looks promising. I tried to run this branch but I couldn't get it to run. Is it based on an old version of Gutenberg?

getdave · 2022-12-07T16:08:34Z

From watching the video this looks really great 👏 Nice work!

As @draganescu said let's be sure to sync the actual animation timings and functions with those used in Global Styles panels.

I'll take a look at the implementation to see if I can come up with ideas to decouple the blocks from the block-editor.

getdave · 2022-12-07T16:17:27Z

packages/block-editor/src/components/block-inspector/index.js

+	const { parentNavBlockClientId } = useSelect( ( select ) => {
+		const { getSelectedBlockClientId, getBlockParentsByBlockName } =
+			select( blockEditorStore );
+
+		const _selectedBlockClientId = getSelectedBlockClientId();
+
+		return {
+			parentNavBlockClientId: getBlockParentsByBlockName(
+				_selectedBlockClientId,
+				'core/navigation',
+				true
+			)[ 0 ],
+		};
+	}, [] );


In #46058 I tried to make the whole "Back button" thing universal with the only exceptions being the Template Part and Reusable Blocks.

If we were to persue that route we could remove this code and make it generic to the "nearest controlling parent" right?

Another option might be to investgate whether we can configure the animation based on information passed via block editor settings.

Here's some prior art on that one.

So you could have a setting which determines the specific blocks that will exhibit the special "back button" behaviour.

This is all to avoid coupling a block to the block-editor package.

artemiomorales · 2022-12-08T04:43:18Z

From the video it looks like a very good start. It's a bit jarring that we have a blank intermission, that is the present screen disappears too quickly and the future one comes from too far away. To guide in terms of animation tweaking use the global styles animation as a reference.

@draganescu Ok I modified the animation to more closely resemble the global styles animation. How does it look now?

The video looks promising. I tried to run this branch but I couldn't get it to run. Is it based on an old version of Gutenberg?

@scruffian Hmm it's been rebased on the latest version of trunk, so it should be working.

Another option might be to investigate whether we can configure the animation based on information passed via block editor settings.

@getdave I added a rough implementation of a block editor setting as you suggested. I'm still trying to make the animation occur only when when the navigation link edit button or back button are pressed.

Overall question: Should the animation still occur when blocks are selected via the Document Overview Panel or the canvas?

artemiomorales · 2022-12-08T15:47:41Z

For reference, this is what the animation looks like at the moment:

animation-edit.mov

getdave

Nice work iterating on this.

I think we should seek input from designers as to whether it's worthwhile to just enable the animation for all panel interactions?

Of course if we need specific directional exceptions for certain blocks then perhaps the config coming from the settings will work well.

@jasmussen might be able to offer an opinion.

getdave · 2022-12-08T16:48:45Z

packages/block-library/src/navigation-link/index.php

@@ -376,3 +376,23 @@ function gutenberg_disable_tabs_for_navigation_link_block( $settings ) {
 }

 add_filter( 'block_editor_settings_all', 'gutenberg_disable_tabs_for_navigation_link_block' );
+
+function gutenberg_enable_animation_for_navigation_link_inspector( $settings ) {
+	$current_tab_settings = _wp_array_get(


Does $current_tab_settings need renaming?

Thanks for catching this! That variable has been renamed.

jasmussen · 2022-12-08T16:58:16Z

Can look more in depth tomorrow and review, but my first instinct is that this should use the same component as the global styles inspector uses:

I don't think it's necessarily urgent that this happen, but it seems like we should avoid drift? I recall there being a ton of focus issues needing to be worked out for the Navigate component, it'd be good to avoid those by rebuilding it.

scruffian · 2022-12-08T17:59:30Z

I think the animations are happening in the wrong direction. In Global Styles the panels enter from the right and leave to the right, whereas here they enter and leave from the left.

artemiomorales · 2022-12-08T20:16:07Z

I think the animations are happening in the wrong direction. In Global Styles the panels enter from the right and leave to the right, whereas here they enter and leave from the left.

@scruffian Thanks for catching this. I accidentally switched it as I was cleaning things up yesterday; the animation should move in the right direction now.

Can look more in depth tomorrow and review, but my first instinct is that this should use the same component as the global styles inspector uses.

I don't think it's necessarily urgent that this happen, but it seems like we should avoid drift? I recall there being a ton of focus issues needing to be worked out for the Navigate component, it'd be good to avoid those by rebuilding it.

@jasmussen If we decide to go through with this experiment, using the <Navigator> component and modifying it for this use case may be worth looking into. I did try using it already though and my impression was that it doesn't work well with the block inspector logic.

If we do use it, I would want us to modify it so use of its history stack is optional, as in this scenario we only need the animation logic. But with that being the case, I think accessing the Framer Motion functionality directly, coupled with a standard way for implementing animations via block settings, may be an alternative way to prevent drift as well as keep the implementation simple.

The <Navigator> could still play a role in that, though its requirement to contain all JSX to be declared upfront seems relatively inflexible and difficult to implement for this use case at the moment. Would love to hear any additional suggestions or thoughts folks may have.

I also haven't looked at accessibility considerations with this animation experiment yet, so to @jasmussen's point, there may be additional points to consider there.

Additional Thoughts

I believe this pull request may be at the stage that we can use it for user testing. Please note though that lint-js fired the following errors for me, which I overrode for now. Is it alright to go ahead with these errors for the purpose of this experiment, or should these be remedied?

  24:1   error  'framer-motion' import is restricted from being used. Please use the Framer Motion API through `@wordpress/components` instead  no-restricted-imports
  24:1   error  'framer-motion' should be listed in the project's dependencies. Run 'npm i -S framer-motion' to add it                          import/no-extraneous-dependencies

Currently, the animation is only visible in the Site Editor. Should I implement it in the Post Editor as well?
Should I look into accessibility considerations?

jasmussen · 2022-12-09T09:15:33Z

Thank you for the clarity Artemio, much appreciated. I can't speak too deeply into the underlying technology and will refer to you and others, though I would love input from @youknowriad or @jorgefilipecosta if they have time. The key concen I want to voice is:

This experiment is so far working great, I would expect it to graduate.
For that reason, we should think in terms of re-usable componentry across blocks. For example I expect this same innerblock-drilldown mechanism to apply to contentOnly locked patterns.
Since we already have a Navigator component for global styles, it seemed like the better long term approach to refine that component and reuse it here, rather than write new unique code.

youknowriad · 2022-12-09T09:20:40Z

Personally I tend to agree with @jasmussen here that we should try to consolidate by using the Navigator components. If we're not using it here which seems to me like a perfect fit, it may mean the component is not good enough (or there's no point in having it in the first place).

I think we already discussed some of that in one of @getdave's navigation block off canvas follow-up issues. and yeah it doesn't have to be in this PR.

scruffian · 2022-12-09T13:20:30Z

Thanks for all your work on this.

Please note though that lint-js fired the following errors for me, which I overrode for now. Is it alright to go ahead with these errors for the purpose of this experiment, or should these be remedied?
24:1 error 'framer-motion' import is restricted from being used. Please use the Framer Motion API through @wordpress/components instead no-restricted-imports
24:1 error 'framer-motion' should be listed in the project's dependencies. Run 'npm i -S framer-motion' to add it import/no-extraneous-dependencies

You should change the dependencies as suggested in the error, and then it will go away.

Currently, the animation is only visible in the Site Editor. Should I implement it in the Post Editor as well?

I think that's ok as a known limitation, given that this is purely for the sake of running a user test.

Should I look into accessibility considerations?

Again, given that this is purely for the sake of running a user test, I think we can delay addressing these until we are ready to commit to the experiment.

It would be good to followup on the original issue (#45884) that we have implemented this as a quick fix but that it needs revisiting, and giving details on what the known limitations are.

artemiomorales · 2022-12-09T14:48:17Z

@scruffian Great, the dependency errors have been fixed. I'll change the PR from 'draft' to 'ready for review.'

Personally I tend to agree with @jasmussen here that we should try to consolidate by using the Navigator components. If we're not using it here which seems to me like a perfect fit, it may mean the component is not good enough (or there's no point in having it in the first place).

I think we already discussed some of that in one of @getdave's navigation block off canvas follow-up issues. and yeah it doesn't have to be in this PR.

@youknowriad Got it. As per @scruffian's suggestion, I can follow up with additional thoughts and we can continue the discussion related to the Navigator in the original issue #45884

scruffian · 2022-12-12T11:06:42Z

Looks like you have some linting errors that need addressing.

artemiomorales · 2022-12-13T22:57:38Z

@scruffian All checks have passed. Just let me know if this needs anything else!

getdave

First up great work on finding an expedient solution to the immediate problem of animation.

That said it's a tricky one, because if this gets merged then it's a change that's bleeding outside of the nav offcanvas experiment by getting into the block settings as an experimental prop which will then need to be managed and removed/stabilised by the time WP 6.2 comes around.

As has been suggested we could use the <Navigator> component and extend it to be used in the inspector controls, but that would still leave the problem that we probably don't necessarily want to apply this animation to all inspector controls.

In the interests of expediency (i.e. improving the Nav offcanvas experiment) I'm in favour of merging this one but on the following provisos:

@artemiomorales and the contributors working on the nav offcanvas experiment commit to managing the __experimentalBlockInspectorAnimation property for the WP 6.2 release cycle.
we follow up to investigate the possibility of utilising <Navigator> in the Inspector Controls as suggested by several folks here and elsewhere.

What do we all think?

Thanks again @artemiomorales for continuing to work on this one 🙇

getdave · 2022-12-14T09:24:43Z

packages/block-editor/src/components/block-inspector/index.js

+			}
+			return null;
+		},
+		[ selectedBlockClientId ]


blockType.name is used within the select but is not listed in the deps.

getdave · 2022-12-14T09:25:10Z

packages/block-editor/src/components/block-inspector/index.js

+
+	const blockInspectorAnimationSettings = useSelect(
+		( select ) => {
+			if ( isOffCanvasNavigationEditorEnabled ) {


This is used within the select but is not in the useSelect dependencies.

getdave · 2022-12-14T09:27:11Z

packages/block-editor/src/components/block-inspector/index.js

+				<BlockInspectorSingleBlock
+					clientId={ selectedBlockClientId }
+					blockName={ blockType.name }
+				/>


This block of code is duplicated as the default return statement.

Could we consider conditionally wrapping the default return in the motion div and avoid the duplication?

scruffian

This is looking good to me. Just a couple of cleanup things Dave suggested and I think we can merge it.

artemiomorales · 2022-12-14T15:51:31Z

@scruffian @getdave Ok I added a wrapper for the motion div to avoid duplication and added the dependencies to the useSelect. How does that look?

Currently trying to get all the checks to pass.

…tion implementation

scruffian

I think this is good for a quick fix for now. Let's get it in and then look at what it would take to refactor it to use the Navigator component next.

* Add rough animation to navigation and links * Add support for configuring animation in block settings; revise animation implementation * Fix animation direction; do code cleanup * Add framer dependency and disable import restriction * Fix linter errors * Limit scope to fix tests * Add wrapper for animation logic, update useSelect dependencies * Fix dependency declaration that was causing tests to break

getdave reviewed Dec 7, 2022

View reviewed changes

artemiomorales force-pushed the try/animate-off-canvas-editor branch from 06e06dc to a2014e0 Compare December 8, 2022 04:29

getdave reviewed Dec 8, 2022

View reviewed changes

artemiomorales force-pushed the try/animate-off-canvas-editor branch from a2014e0 to 0286d0e Compare December 8, 2022 19:32

artemiomorales force-pushed the try/animate-off-canvas-editor branch from 0286d0e to fbeeefa Compare December 9, 2022 14:38

artemiomorales marked this pull request as ready for review December 9, 2022 14:48

artemiomorales requested review from TimothyBJacobs, spacedmonkey, geriux, ellatrix and tellthemachines as code owners December 9, 2022 14:48

artemiomorales mentioned this pull request Dec 9, 2022

Use <Navigator> component on Inspector Controls "back" button UX #45884

Open

draganescu assigned artemiomorales Dec 12, 2022

artemiomorales force-pushed the try/animate-off-canvas-editor branch from fbeeefa to dced819 Compare December 12, 2022 19:50

scruffian enabled auto-merge (squash) December 12, 2022 23:20

auto-merge was automatically disabled December 12, 2022 23:37
Head branch was pushed to by a user without write access

artemiomorales force-pushed the try/animate-off-canvas-editor branch from dced819 to aca880d Compare December 12, 2022 23:37

artemiomorales force-pushed the try/animate-off-canvas-editor branch from aca880d to 61a1339 Compare December 13, 2022 14:29

getdave reviewed Dec 14, 2022

View reviewed changes

scruffian reviewed Dec 14, 2022

View reviewed changes

getdave added [Package] Block editor /packages/block-editor [Type] Experimental Experimental feature or API. labels Dec 14, 2022

artemiomorales force-pushed the try/animate-off-canvas-editor branch from 2417c87 to 4cdcdce Compare December 14, 2022 15:14

artemiomorales added 8 commits December 14, 2022 11:54

Add rough animation to navigation and links

f733376

Add support for configuring animation in block settings; revise anima…

071f6ed

…tion implementation

Fix animation direction; do code cleanup

2c0f1a3

Add framer dependency and disable import restriction

f285821

Fix linter errors

9d4b367

Limit scope to fix tests

4379d9d

Add wrapper for animation logic, update useSelect dependencies

33f6bda

Fix dependency declaration that was causing tests to break

b7f3b68

artemiomorales force-pushed the try/animate-off-canvas-editor branch from 4cdcdce to b7f3b68 Compare December 14, 2022 16:55

scruffian approved these changes Dec 14, 2022

View reviewed changes

scruffian merged commit b856a3f into WordPress:trunk Dec 14, 2022

github-actions bot added this to the Gutenberg 14.9 milestone Dec 14, 2022

getdave mentioned this pull request Dec 15, 2022

Fix accessing potentially undefined block editor setting __experimentalBlockInspectorAnimation #46570

Merged

artemiomorales mentioned this pull request Dec 16, 2022

Add Navigator to off-canvas block inspector #46617

Draft

ntsekouras mentioned this pull request Jan 17, 2023

Audit Experimental APIs for WP 6.2 #47196

Closed

14 tasks

Mamaduka mentioned this pull request Feb 1, 2023

Block Library: Don't use methods with the 'gutenberg_' prefix #47649

Merged

ntsekouras mentioned this pull request Feb 3, 2023

[Block Editor]: Lock blockInspectorAnimation setting #47740

Merged

draganescu mentioned this pull request Feb 6, 2023

All navigation block children should be animated #47784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rough animation to navigation and links #46342

Add rough animation to navigation and links #46342

artemiomorales commented Dec 7, 2022 •

edited

Loading

draganescu commented Dec 7, 2022

scruffian commented Dec 7, 2022

getdave commented Dec 7, 2022

getdave Dec 7, 2022

getdave Dec 7, 2022

artemiomorales commented Dec 8, 2022

artemiomorales commented Dec 8, 2022

getdave left a comment

getdave Dec 8, 2022

artemiomorales Dec 8, 2022

jasmussen commented Dec 8, 2022

scruffian commented Dec 8, 2022

artemiomorales commented Dec 8, 2022

jasmussen commented Dec 9, 2022

youknowriad commented Dec 9, 2022

scruffian commented Dec 9, 2022

artemiomorales commented Dec 9, 2022

scruffian commented Dec 12, 2022

artemiomorales commented Dec 13, 2022

getdave left a comment •

edited

Loading

getdave Dec 14, 2022

getdave Dec 14, 2022

getdave Dec 14, 2022

scruffian left a comment

artemiomorales commented Dec 14, 2022

scruffian left a comment

Add rough animation to navigation and links #46342

Add rough animation to navigation and links #46342

Conversation

artemiomorales commented Dec 7, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Additional Notes and Question for Discussion

Screen shots or screencast

draganescu commented Dec 7, 2022

scruffian commented Dec 7, 2022

getdave commented Dec 7, 2022

getdave Dec 7, 2022

Choose a reason for hiding this comment

getdave Dec 7, 2022

Choose a reason for hiding this comment

artemiomorales commented Dec 8, 2022

artemiomorales commented Dec 8, 2022

getdave left a comment

Choose a reason for hiding this comment

getdave Dec 8, 2022

Choose a reason for hiding this comment

artemiomorales Dec 8, 2022

Choose a reason for hiding this comment

jasmussen commented Dec 8, 2022

scruffian commented Dec 8, 2022

artemiomorales commented Dec 8, 2022

Additional Thoughts

jasmussen commented Dec 9, 2022

youknowriad commented Dec 9, 2022

scruffian commented Dec 9, 2022

artemiomorales commented Dec 9, 2022

scruffian commented Dec 12, 2022

artemiomorales commented Dec 13, 2022

getdave left a comment • edited Loading

Choose a reason for hiding this comment

getdave Dec 14, 2022

Choose a reason for hiding this comment

getdave Dec 14, 2022

Choose a reason for hiding this comment

getdave Dec 14, 2022

Choose a reason for hiding this comment

scruffian left a comment

Choose a reason for hiding this comment

artemiomorales commented Dec 14, 2022

scruffian left a comment

Choose a reason for hiding this comment

artemiomorales commented Dec 7, 2022 •

edited

Loading

getdave left a comment •

edited

Loading