-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigation: Move manage menus button to the navigation selector #45785
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
Size Change: -42 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
{ isOffCanvasNavigationEditorEnabled && ( | ||
<ManageMenusButton | ||
disabled={ isManageMenusButtonDisabled } | ||
className="wp-block-navigation-manage-menus-button" | ||
/> | ||
) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to test this branch of code, but from the code it looks like this instance of the button is removed entirely. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the button now always lives in the menu selector, so we don't need it down here as well.
Looks good to me @scruffian ! |
I'm not able to test this PR at the moment, but thank you for it, IMO this strikes a better balance of prominence for the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR moves the manage menus for the offcanvas editing experiment too. For that, the manage menus should remain in the advanced area as design suggested.
Also, I think, even outside the experiment, moving manage menus to the menu selector needs design feedback, so this is not just a refactoring.
If design feedback is to move it to the selector, we need to figure it out since one direction is to hide it into oblivion and another to make it easily available 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts on this...
<MenuItem | ||
disabled={ ! canManageMenus } | ||
href={ addQueryArgs( 'edit.php', { | ||
post_type: 'wp_navigation', | ||
} ) } | ||
> | ||
{ __( 'Manage menus' ) } | ||
</MenuItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This UI is effectively representing a standard <select>
component. I don't know for certain but my instinct is that it's not going to be accessible for what is effectively a standard link to be inside the select.
That said, when it's inside the experiment that's a different matter. In that case the link will be inside a dropdown from the panel's 3 dots menu which I think will be fine.
As far as design feedback goes, I personally think the dropdown is a good place for this. It's contextual to the other menus and tools, and it manages prominence. Besides, you can both delete and rename menus on the Manage page (although I could see the Manage page getting a much needed upgrade separately). This is also where the "Manage" link started, if I recall correctly. However the above is not a strong opinion. I'm also am aware of the recent conversation around moving it to "Advanced" instead, and what either of those placements (menu or advanced) boil down to, is so that we don't show it here: So long as that's the case, I don't personally think it's urgent to move the link in either direction. |
Thanks for the feedback, lets table this until we have an outcome from the experiment. |
What?
This moves the "Manage menus" button inside the Navigation Menu Selector component
Why?
This feels like a more natural home for this link since it relates directly to the contents of the selector.
How?
This is a small refactoring since the previous code tidyup made things cleaner.
Testing Instructions
Screenshots or screencast