Navigation: Select dropdown encapsulation and further consolidation. #38627
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
getdave
added
[Type] Code Quality
|
Size Change: +3.88 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
talldan
reviewed
Feb 9, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Feb 9, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Feb 9, 2022
adamziel
reviewed
Feb 9, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Feb 9, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
8 tasks
|
Here's the artifact from the failing e2e test. Notice how the menu is appearing to be in the placeholder state even though the test has already selected the classic menu. I think this might be because we assume the selection of the classic menu is "done" once the button is clicked whereas in fact it relies on the network. Therefore we probably need to wait for the link blocks to appear in the Nav block before saying "we're done". |
|
Only thing left is to work out why the e2e test has started failing. FYI it passes locally so it's likely a network issue with the loading of the menu data. |
adamziel
reviewed
Feb 14, 2022
| canUserCreateNavigation: canUserCreateNavigationMenu, | ||
| canUserUpdateNavigationEntity: canUserUpdateNavigationMenu, | ||
| canSwitchNavigationMenu, | ||
| } = useNavigationMenu(); |
There was a problem hiding this comment.
What if the hook returned the final names already?
There was a problem hiding this comment.
It doesn't...yet but yes this will require a fix when my other PR lands.
adamziel
reviewed
Feb 14, 2022
Comment on lines
116
to
118
| { showManageActions && | ||
| ( canUserCreateNavigationMenu || | ||
| canUserUpdateNavigationMenu ) && ( |
There was a problem hiding this comment.
that's quite a condition :D how about showManageActions && hasManagePermissions?
|
This is fantastic, thank you so much @getdave – your work makes the block more readable and less error prone by the day! |
adamziel
approved these changes
Feb 14, 2022
cbravobernal
changed the title
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.
Description
This PR builds on #38179 to further consolidate and normalize all code paths around the "Select menu" dropdown that is shown in:
This PR moves all permissions checking and data fetching within the
<NavigationMenuSelector>component and then utilises that single component in both instances (as above).This ensures all logic is the same across all areas where this UI is exposed.
Also addresses part of #37190.
Testing Instructions
The aims is that nothing changes for the block UI. It should be "as was".
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).