Adds all allowed innerblocks to the inspector animation experiment #47834
Conversation
draganescu
changed the title
draganescu
added
[Type] Enhancement
|
Size Change: +115 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
draganescu
added
the
Backport to WP 6.7 Beta/RC
There was a problem hiding this comment.
Thank you @draganescu, but this change enables animation to these blocks even if they are not children of Navigation block. This deviates from the current behavior and maybe the blockInspectorAnimation API needs to be revisited.
I don't think we should land this..
|
@ntsekouras I did mention that in the "Caveats" headline in the PR description. The navigation children that do animate have the "advantage" to only be used inside the navigation block. But the experience of using the block tree is not great at all without this navigation. We dropped the edit button that appeared on hover also because this animation made the jarring replacement of the entire inspector on click less jarring. @SaxonF which one is it worth more doing:
|
Sorry, I missed that. @draganescu it seems to me that a better option is to revisit the |
This approach makes sense to me. |
scruffian
force-pushed
the
add/animation-to-all-navigation-items
branch
from
7006f99 to
7f7ef13
Compare
|
Flaky tests detected in 4d6ad96. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4244555093
|
I added a commit to handle this. @draganescu what do you think of this approach? |
| @@ -172,9 +172,17 @@ export const SETTINGS_DEFAULTS = { | |||
|
|
|||
| // This setting is `private` now with `lock` API. | |||
| blockInspectorAnimation: { | |||
| animationParent: 'core/navigation', | |||
There was a problem hiding this comment.
I haven't dug deep into blockInspectorAnimation API. Is it an API that we will reuse in other places and blocks? If yes, animationParent(or whatever the final API will be), should be inside each block, like enterDirection is. If not, maybe it's something to be implemented differently and more close to Navigation block? 🤔
There was a problem hiding this comment.
It's only used for the navigation block at the moment. I am aware that this implementation won't scale well, but since it's locked I wasn't too worried about that. We can improve it when we need it to do more.
There was a problem hiding this comment.
If this API is tightly coupled with Navigation block, have you explored a way to handle this 'closer' to the block? Also you have explored and still think it should be here, should we rename to reflect that? blockInspectorAnimation is definitely a generic name, that may create confusion I think..
There was a problem hiding this comment.
This APi should work for all blocks that have children which are controlled by the parent in various ways. For now the two examples are navigation block and the pattern block in content only mode.
It is not an API specific to the navigation block and it should not be at the block level.
@ntsekouras which part do you think should be extracted to a hook?
|
Why do I feel that the block inspector should be animated on all occasions and have that animation disabled if reduceMotion is on? Do we really need an API to specify animate this block in this situation? Why not always have the previous inspector exit to left and new inspector enter via right? cc @SaxonF |
@draganescu I'm not sure I fully understand the conversation here so apologies if I don't answer your question. The animation should be used to illustrate parent <> child relationship, much like the left sidebar navigation (e.g. clicking templates). This might be a common interaction in future beyond just the navigation block. |
|
@SaxonF why not always use animation to show inspector panels? For instance when selecting a block and then selecting another block? Also, if that is a bad idea, whould we try always use animation when selecting a block that has a parent other than root? |
I was thinking about parent <> child relationship from a panel perspective rather than block. We use animation to signify the ability to move back up a level. Global styles inspector uses the same pattern. I do understand it isn't a perfect pattern though as you can select children within the layers sidebar and still see the navigation (and back button). I'm sure this is something we can continue iterating on. |
|
@draganescu I agree that we should always have the animation, and I'd like to always have the back button too, but we should make that deliberate choice! |
There was a problem hiding this comment.
I'm happy with this. @draganescu if you're happy with my changes we can merge!
| @@ -179,6 +179,30 @@ const BlockInspector = ( { showNoBlockSelectedMessage = true } ) => { | |||
| const globalBlockInspectorAnimationSettings = | |||
| select( blockEditorStore ).getSettings() | |||
| .blockInspectorAnimation; | |||
|
|
|||
| // Get the name of the block that will allow it's children to be animated. | |||
| const animationParent = | |||
There was a problem hiding this comment.
Related to my other comments about the API, if we leave this here, maybe it's better to extract this to a separate hook. What do you think?
|
If we want this to be for all parent child relationships it shouldn't even be an opt in API, it should just check that we're in a parent - child selection and just work. We then should not need to hardcode names of blocks like we do here, but just be aware if block's have parents. |
|
After more research into what is it exactly that we're after, I now see that the block inspector animation API is a means of allowing a way for the UI to better represent the relationship between children blocks and the parents that control them. Not all block parents are controlling, some are. For instance the navigation block and the pattern block in content only locking mode could be considered to be controlling their children We don't have a definition yet for this aspect "which block is controlling its children and which doesn't". As we can see from the pattern block example not all instances of that block are "controlling" - only when the locking is set to "content only". Therefore for the sake of the experience in 6.2 and given that this is exclusively internal code that nobody relies on and can later be discarded I would merge this as is with @scruffian 's quick fix - so that allowed children of the navigation block do not animate in other contexts - and then come back to it once we have a way to designate (via a config list, via a flag, via some inference mechanism) which parent is controlling and which isn't. @ntsekouras do you agree with this? Do you see any major issue with continuing with this as a sort of UI bug fix for the currently jarring experience in the navigation block's inspector in 6.2? |
|
Since the goal ended up being a generic API, I'm fine with merging this for now with a couple of things:
Sounds good? |
Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
draganescu
force-pushed
the
add/animation-to-all-navigation-items
branch
from
c364cbe to
4d6ad96
Compare
|
@ntsekouras sounds excellent applied all in 4d6ad96 👏🏻 Let me know if I can do more to help this land 🙇🏻 |
| @@ -56,7 +57,7 @@ function useContentBlocks( blockTypes, block ) { | |||
| ( blockName ) => { | |||
| return !! contentBlocksObjectAux[ blockName ]; | |||
| }, | |||
| [ blockTypes ] | |||
There was a problem hiding this comment.
This is something exhaustive deps linter complained about, and it was right.
…47834) * adds all allowed innerblocks to the inspector animation experiment * don't animate unless we're in a navigation block * make the nav block a config * Update packages/block-editor/src/components/block-inspector/index.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> * remove redundant comment * be more clear block animation is temporary --------- Co-authored-by: scruffian <ben@scruffian.com>
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: 67ce029 |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
What?
Closes #47784
Why?
For consistency.
How?
Adds all allowed innerblocks to the inspector animation experiment.
Testing Instructions
Testing Instructions for Keyboard
Caveats
I learned that the way this experiemnt works the block inspector of all the blocks in the experiment is animated irrespective or whether they are a child of a navigation block or not.