Navigation: Remove the check for draft navigation menus from the UnsavedInnerBlocks component #49161
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
|
Size Change: -31 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
getdave
reviewed
Mar 17, 2023
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
scruffian
force-pushed
the
remove/check-for-draft-navigation-menus
branch
from
461d4c6 to
623af6a
Compare
|
@scruffian I looked at this again and saw this line:
I assume we need all menus so when the menu name is created it isn't a duplicate? This would only be a factor however if the code in Looking I can see that Pinging @talldan as well because I feel like he might have written this component originally. |
|
Noting also that we have some e2e test coverage for this functionality now. I guess we could amend that to prepopulate with a draft and a published menu and then assert that when inner blocks change only a single menu gets created and that it's title doesn't clash with the other ones. |
I think it's just because it has been refactored, and some dead code wasn't removed. In the past the menu creation/naming code was inside |
|
I wondering if we could change the resolution checker to check for both publish and draft? That way if the code in the |
getdave
reviewed
Mar 31, 2023
|
The problem with the approach in this PR is that adding the page list as fallback ends up loading the unsaved inner blocks component which then ends up creating a wp_navigation menu. I think we should wait for #48698 to land and then reasses what we should do here... |
I don't understand why that would happen. It should (and did!) happen like this:
|
getdave
force-pushed
the
remove/check-for-draft-navigation-menus
branch
from
4627173 to
277b515
Compare
getdave
approved these changes
May 11, 2023
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?
I was trying to track this down in #49143 but I was looking in the wrong place! This removes a check for loaded draft navigation menus from the
UnsavedInnerBlockscomponent.Why?
I believe it is unnecessary to make this request, since we are already fetching both draft and published navigations in the
useNavigationMenuhook.How?
Deleting code!
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Network tab with only one GET:
