Refactor away state in Nav menu selector #45464
Merged
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
commented
Nov 1, 2022
|
|
||
| if ( isCreatingMenu || isResolvingNavigationMenus ) { |
There was a problem hiding this comment.
Made this loading state conditional on actually being in a resolving state rather than whether we have resolved or not.
getdave
commented
Nov 1, 2022
getdave
commented
Nov 1, 2022
|
|
||
| let enableOptions = false; | ||
|
|
||
| if ( isCreatingMenu ) { |
There was a problem hiding this comment.
This condition was dotted about. I've centralise it here. If we are creating then never show the options.
getdave
commented
Nov 1, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
getdave
commented
Nov 1, 2022
getdave
commented
Nov 1, 2022
getdave
commented
Nov 1, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
|
Size Change: +586 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
4 tasks
getdave
force-pushed
the
try/refactor-away-state-in-nav-menu-selector
branch
from
82f9963
to
dea3e92
Compare
|
I refactored this using the unit tests to check the behaviour remains the same We should verify the behaviour is correct with manual testing just to ensure the tests cover all the scenarios. |
scruffian
reviewed
Mar 2, 2023
| }, [ | ||
| isCreatingMenu, | ||
| createNavigationMenuIsError, | ||
| createNavigationMenuIsSuccess, |
There was a problem hiding this comment.
Should we also add setIsCreatingMenu as a dependency?
There was a problem hiding this comment.
useState setters are guaranteed by React to display referential equality. I think this is because they derive from dispatch which is the same. I can double check that but I'm pretty sure it's correct.
There was a problem hiding this comment.
I don't think we need to pass it as a dependency because we think it might change, only because we have a warning about undeclared dependencies.
There was a problem hiding this comment.
I committed a change. Does that work?
getdave
commented
Mar 2, 2023
getdave
commented
Mar 2, 2023
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in 90b6737. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4312930770
|
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?
Aims to refactor away the use of setState in favour of computed values in order to simplify the logic and improve the handling in the Navigation block's menu selector within the inspector controls.
Why?
The current code is
How?
This PR aims to:
Testing Instructions
Screenshots or screencast