Reset the persisted selected menu ID if there are no longer any menus #32306
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
[Feature] Navigation Screen
[Type] Bug
|
Size Change: +32 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
talldan
reviewed
May 28, 2021
There was a problem hiding this comment.
So I guess the 404 I was seeing from trying to fetch a non-existent menu?
Does this code prevent that? I'm not sure if it needs to, it might add additional loading time if all menus need to be fetched before fetching the selected menu, so perhaps it's better to let it 404.
Though another thought -an alternative might be that the code to reset the selected menu id happens as a result of the REST request for fetching the selected menu returning 404.
| const selectedMenu = menus?.find( ( { id } ) => id === selectedMenuId ); | ||
|
|
||
| if ( ! menus?.length || ! selectedMenu ) { | ||
| setSelectedMenuId( 0 ); |
There was a problem hiding this comment.
I feel like null might better represent the absence of something. I know WordPress likes to count from 1, but 0 still seems too much like a legit menu id.
There was a problem hiding this comment.
I tend to agree. I was basing it on the default state:
But I guess if we pass null then it will reset to 0 anyway.
Alternatively, we could create a new action CLEAR_SELECTED_MENU_ID which might make it more explicit about what the intention is.
There was a problem hiding this comment.
Yes, 404 originates from the Header component when it tried to fetch menuName. This code won't prevent that but will fix the broken screen issue.
We currently don't have a way to reset the selected menu ID if menus were deleted by any other means, e.g., WP CLI.
I agree that changing the default/cleared state to be null makes more sense now. I think @getdave mentioned this when reviewing original PR.
There was a problem hiding this comment.
I have an alternative PR that we also might want to consider. It's incoming...
There was a problem hiding this comment.
Alternative approach available #32313
7 tasks
|
This approach is probably better #32313 |
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
In #31320 we started persisting the
selectedMenuIdto localStorage. Code was in place to reset this ID if the menu was deleted. However, there were no guards against the following conditionsThis PR attempts to address this.
Fixes #32256
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).