-
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
Add classic menus to menu switcher #38168
Conversation
Size Change: +44 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
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.
Thanks for the quick fix! Code looks good and dropdown is working well 👍
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.
The code looks good and it tests well for me locally 👍
I double checked that we're not introducing any new strings 👍
I think it's a good solution for 5.9 given the time and string constraints 👍
I think the Tools heading might be unnecessary. Your call.
} ) } | ||
</MenuGroup> | ||
) } | ||
<MenuGroup label={ __( 'Tools' ) }> |
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.
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 think I'll leave it. To me it feels cleaner to me to consistently have headings when there are multiple groups like this.
If only there were designers in this timezone we could ask 😄
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 also like using different groups for Classic and Block menus.
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.
Cool, your call.
If only there were designers in this timezone we could ask 😀
😅 yes unfortunately there wouldn't be enough time to do the backports if we wait. I put #36311 into the 5.9.x project board so that we can iterate asap.
packages/block-library/src/navigation/use-convert-classic-menu.js
Outdated
Show resolved
Hide resolved
* Add classic menus to switch menu dropdown * Preload entities * Hide classic menus when there are none * Memoize callback
Tested the backport of this fix in Core's 5.9 branch and can confirm this does fix the issue to unblock users upgrading to 5.9 and a block theme. Amazing work @talldan 👏 🌟 Thank you everyone for quickly resolving this user workflow issue! |
Description
Fixes #38166
How has this been tested?
Expected: The classic menu should be converted to a block menu
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).