Navigation block - minor refactor to classic menu conversion code #43081
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
talldan
added
[Type] Code Quality
|
Size Change: -35 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
…the convertClassicMenu function
talldan
force-pushed
the
update/convert-classic-menu-to-return-a-result
branch
from
729d5d5
to
8a015b1
Compare
| onSelectClassicMenu={ ( classicMenu ) => { | ||
| convert( classicMenu.id, classicMenu.name ); | ||
| setShouldFocusNavigationSelector( true ); | ||
| onSelectClassicMenu={ async ( classicMenu ) => { |
There was a problem hiding this comment.
Would it be worth extracting this to a shared method since it is the same as the above event handler?
There was a problem hiding this comment.
Thanks for reviewing. The final code is different in #42956, where there will be an extra line in one of the callbacks.
Extracting to another function is still possible, but I'm not sure it's worth it for only a few lines. I personally prefer the improved readability gained by not having to scroll to the definition.
glendaviesnz
approved these changes
Aug 15, 2022
There was a problem hiding this comment.
Worked as advertised for me. Was able to:
✅ Add a Navigation block and from the block toolbar, use the 'Select menu' dropdown to select a classic menu and the classic menu was created.
✅ Use the toolbar's 'Select menu' dropdown again and click 'Create new menu'
then from the placeholder use the 'Select menu' dropdown and select a classic menu and it was created.
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?
Originally part of #42956, split into a seperate PR as requested
This updates the
convertfunction that's returned from theuseConvertClassicToBlockMenuhook so that it returns a promise that resolves to the createdwp_navigationmenu.Why?
Previously, the code used an effect to handle the result of classic menu conversion.
This posed a problem for the changes in #42956. In my PR, I need to handle the outcome differently depending on whether the user converted a menu from the placeholder or whether they converted a menu from the block toolbar. The former calls
selectBlockas it does intrunk, but the latter doesn't (as doing so causes a focus loss).With the effect, there was no way to determine the source of the conversion. In this PR the code can
awaitthe menu conversion and then either select the block or not in the event handler.Testing Instructions
(Before testing, ensure you have some classic menus)