Alternative fix: set persisted menu id when no menus or missing menu #32313
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
0 is a valid menuID so we had requests going out to `/menus/0` which always 404’d. By using `null` as the default we can reset a selected menu ID and ensure that: 1. No requests go out. 2. We’re clearer that we have no menu set.
getdave
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
7 tasks
|
Size Change: +67 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
getdave
changed the title
|
I tested PR by following instructions, and it works as expected. I can see two 404 requests to What will be the downside if we check menu existence inside the Example code: export const getSelectedMenuId = createRegistrySelector(
( select ) => ( state ) => {
const menuId = state.selectedMenuId ?? null;
const menu = menuId
? select( coreStore ).getEntityRecord(
MENU_KIND,
MENU_POST_TYPE,
menuId
)
: null;
if ( ! menu ) {
return null;
}
return menu.id;
}
);I think PR looks good, and I wasn't able to spot any obvious issues code-wise. |
@Mamaduka I'm not 100% but it just feels like querying for menus within a selector which is just supposed to retrieve the selected menu isn't something I'd expect to happen. I'd decided to be more declarative and actually make it obvious when and where we're querying for menus rather than hiding it in and amongst other APIs. I'd be interested to hear what other folks think though... |
draganescu
approved these changes
Jun 3, 2021
draganescu
deleted the
fix/alternative-reset-persisted-menu-id-when-no-menus
branch
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
Alternative to #32306.
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.
Moreover, it resolves the edge case whereby due to a default of
0for selectedMenuId, requests were being made to/menus/0for a menu that could never exist. Usingnullas the default prevents this.Fixes #32256
How has this been tested?
Manually only - needs e2e and unit tests.
Manual Instructions
Menu 1andMenu 2.Menu 2. Make a note of it's menu ID by checking the state or network requests.Menu 2.Menu 1becomes selected in the Navigation Editor./menus/{{MENU_ID_OF_MENU_2}}in the Network tab./menus/0in the Network tab.Menu 1./menus/{{MENU_ID_OF_MENU_1}}in the Network tab./menus/0in the Network tab.It should be impossible to arrive at the state observed in #32256.
Screenshots
Screen.Capture.on.2021-05-28.at.12-53-19.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).