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.

Sign up for GitHub

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

Jump to bottom

Site Editor: Fix deprecation console error in top toolbar #55678

Merged
merged 6 commits into from Nov 3, 2023
Merged

Site Editor: Fix deprecation console error in top toolbar #55678

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
59f7fdd
Site Editor: Fix deprecation console error in top toolbar
t-hamano Oct 28, 2023
1f36d2f
Merge branch 'trunk' into quality/use-render-prop
t-hamano Oct 31, 2023
952f3d9
use custom component
t-hamano Oct 31, 2023
cccb544
Wrap `NavigableToolbar` in a `motion.div`
t-hamano Nov 1, 2023
b0889c1
Merge branch 'trunk' into quality/use-render-prop
t-hamano Nov 1, 2023
6be77e0
Merge branch 'trunk' into quality/use-render-prop
t-hamano Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/header-edit-mode/index.js
View file
Open in desktop
Expand Up @@ -191,7 +191,7 @@ export default function HeaderEditMode( { setListViewToggleElement } ) {
>
{ hasDefaultEditorCanvasView && (
<NavigableToolbar
as={ motion.div }
render={ <motion.div /> }
Copy link
Member

Choose a reason for hiding this comment

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

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 } />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@ciampo ciampo Oct 30, 2023 •
edited

Choose a reason for hiding this comment

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

There's a few things happening here:

  • Toolbar technically never supported the as prop: the false here 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 as prop worked (even if technically not supported)
  • Toolbar was recently refactored from reakit to ariakit, which introduced the deprecation warning for the as prop

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

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the other way around?

const MotionNavigableToolbar = motion( NavigableToolbar );
// ...
<MotionNavigableToolbar>

Ref: https://www.framer.com/motion/component/#custom-components

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah we could try that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about wrapping NavigableToolbar in a motion.div ?

Or simply adding the animation via SCSS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about wrapping NavigableToolbar in a motion.div ?

In the end, I did this approach. The layout and animations should remain the same.

className="edit-site-header-edit-mode__start"
aria-label={ __( 'Document tools' ) }
shouldUseKeyboardFocusShortcut={
Expand Down