Site Editor: Fix deprecation console error in top toolbar #55678
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
t-hamano
added
[Type] Code Quality
|
Size Change: -3 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
diegohaz
reviewed
Oct 28, 2023
| @@ -191,7 +191,7 @@ export default function HeaderEditMode( { setListViewToggleElement } ) { | |||
| > | |||
| { hasDefaultEditorCanvasView && ( | |||
| <NavigableToolbar | |||
| as={ motion.div } | |||
| render={ <motion.div /> } | |||
There was a problem hiding this comment.
I don't know if the as prop was part of the public API of the NavigableToolbar component. But if it was, we should probably re-implement that within NavigableToolbar.
Something like this may work:
const Component = props.as;
const render = Component ? <Component /> : undefined;
<Toolbar render={ render } />There was a problem hiding this comment.
Thanks for the review!
It appears that the as prop in the NavigableToolbar component is not defined and is simply received as a remainder of the defined props. In this case, I thought it would be better not to re-implement internally from as prop to render prop, but what do you think?
There was a problem hiding this comment.
There's a few things happening here:
Toolbartechnically never supported theasprop: thefalsehere in fact marks the component as non polymorphic- But since the component internally used to spread the props to the underlying reakit component, passing the
asprop worked (even if technically not supported) Toolbarwas recently refactored fromreakittoariakit, which introduced the deprecation warning for theasprop
So, technically Toolbar doesn't support neither as nor render. On top of that, looking at the implementation of NavigableToolbar, depending on the isAccessibleToolbar prop, it could also render a NavigableMenu, which would ignore the as prop completely.
It just seems to me that the best choice here is to find a different way to animate the header without passing the as prop (or render prop) to Toolbar
There was a problem hiding this comment.
Maybe the other way around?
const MotionNavigableToolbar = motion( NavigableToolbar );
// ...
<MotionNavigableToolbar>Ref: https://www.framer.com/motion/component/#custom-components
There was a problem hiding this comment.
Thank you for your ideas. I have tried various implementations based on the documentation, but so far nothing has worked. The editor crashes with an error logged in the browser console, or the animation doesn't work.
const MotionNavigableToolbar = motion( NavigableToolbar );
// ...
<MotionNavigableToolbar
...
/>const MotionNavigableToolbar = motion( 'NavigableToolbar' );
// ...
<MotionNavigableToolbar
...
/>const MotionNavigableToolbar = motion( 'navigable-toolbar' );
// ...
<MotionNavigableToolbar
...
/>const Component = forwardRef( ( props, ref ) => (
<NavigableToolbar ref={ ref } { ...props } />
) );
const MotionNavigableToolbar = motion( Component );
// ...
<MotionNavigableToolbar
...
/>const Component = forwardRef( ( { children, ...props }, ref ) => (
<NavigableToolbar { ...props } ref={ ref }>
{ children }
</NavigableToolbar>
) );
const MotionNavigableToolbar = motion( Component );
// ...
<MotionNavigableToolbar
...
/>I'll try to find another approach.
There was a problem hiding this comment.
How about wrapping NavigableToolbar in a motion.div ?
Or simply adding the animation via SCSS ?
There was a problem hiding this comment.
How about wrapping
NavigableToolbarin amotion.div?
In the end, I did this approach. The layout and animations should remain the same.
|
Flaky tests detected in b0889c1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6715616142
|
draganescu
approved these changes
Nov 2, 2023
|
Thank you everyone for your review. I found out that this component was refactored in #55770, causing a conflict. So I merged the latest trunk and made similar changes. There are a lot of differences in this PR diff, but I simply wrapped it in I have confirmed that it is working as before, so I would like to merge it as soon as it passes the tests. |
Yup, using the "hide whitespace" option when reviewing the code diff helps a lot to reduce the noise. Code-wise this LTGM, I'll let other folks confirm that the animation behaves as expected. |
Thank you, I didn't know this method until now 😅 |
I didn't know about that either. That's super handy! |
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?
This PR fixes a browser warning error that is logged when opening the editor canvas in the Site Editor.
e370345309f15367d49226c1f3b3bfee.mp4
Why?
According to my research, I found that this error occurred in the
HeaderEditModecomponent. Inside this component, theasproperty is used on theNavigableToolbar. And this component is internally renderingAriakit.Toolbar. As the console message indicates, I expected that we should use therenderproperty instead of theasproperty.How?
I simply changed theWrapasproperty to therenderproperty.NavigableToolbarin amotion.div.Testing Instructions
Screenshots or screencast
22debb53902e0352562fc32b09d35bd7.mp4