Moves the menus state higher in the component tree #22345
Closed
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
draganescu
added
[Type] Bug
|
Size Change: +70 B (0%) Total Size: 833 kB
ℹ️ View Unchanged
|
talldan
requested changes
May 15, 2020
Comment on lines
+13
to
+18
| useEffect( () => { | ||
| if ( wpMenus?.length ) { | ||
| setMenus( wpMenus ); | ||
| setCurrentMenu( wpMenus[ 0 ].id ); | ||
| } | ||
| }, [ wpMenus ] ); |
There was a problem hiding this comment.
@draganescu As mentioned in #22340 (comment), I don't think this code will work, as the deleted menus get restored whenever wpMenus changes.
This effect is listening for changes to wpMenus, and whenver that happens setMenus( wpMenus ) on line 15 effectively undoes any of the changes to menus performed by the onDeleteMenu callback.
There was a problem hiding this comment.
It does currently work and it fixes the problem in #22340, but I agree that keeping a list of deletedItems is better on the long run.
There was a problem hiding this comment.
Yeah, after further debugging I noticed it does usually work, I think it relies on the fact that typically when wpMenus changes, the entity cache has been cleared and so deleted items are no longer present in wpMenus.
There was a problem hiding this comment.
+1, this also affects the menu items themselves - we need one solution to rule them all :-)
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
Closes #22340
How has this been tested?
Tested locally by:
Screenshots
Types of changes
Non breaking change. I moved the menus state in the
Layoutcomponent. This doesn't make me happiest, asLayoutshould not handle this data. However, until we fixcore/datato refresh properly it is a decent solution.