Fix exhaustive deps warning #49291
Merged
Fix exhaustive deps warning #49291
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: -1 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Mar 23, 2023
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in 74613e5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4502096634
|
getdave
reviewed
Mar 30, 2023
There was a problem hiding this comment.
I tested and I didn't see this create more than one menu (i.e. it works 🎉 ).
I really think that before we merge this we need to add a quick e2e test that
- inserts a block with inners blocks as per the test instructions
- adds a new link to the block
- checks that only a single Nav Menu is in the DB.
|
@scruffian I added a test. It involved making some more changes to the test structure and the way we delete menus so pinging @MaggieCabrera and @jeryj who I know worked on these originally. |
This comment was marked as outdated.
This comment was marked as outdated.
getdave
force-pushed
the
fix/deps-warning
branch
from
17a42e1
to
8f942af
Compare
|
@scruffian with the rebase to include @draganescu and my recent e2e tests and the inclusion of a new e2e test in this PR to check for what happens with exisrting inner blocks the e2e tests file was a mess. Therefore I have restructured it which is why you now see a messed up diff. If the tests run and are ✅ we should be ok. |
|
I'm happy with the changes here but I can't approve my own PR! |
getdave
approved these changes
Mar 30, 2023
There was a problem hiding this comment.
My manual test was 👍 and then e2e tests I added in this PR also confirm the expected behaviour of only a single menu being created.
FYI this diff is now a mess because we had several incoming e2e test file changes in trunk which made the test file pretty messy. I ended up resetting the file locally to match all the changes from trunk and then replaying the test I added in this PR on top of that base. Then I reorganised the test structure so it's more logical.
ndiego
added
[Block] Navigation
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?
This adds all the dependencies to this useEffect.
Why?
We tried this once before and it had unexpected side-effects, so we reverted it. The side-effects were fixed in #48219, so we should be good to add this dependency back in.
How?
Just adds the dependency to the dependency array.
Testing Instructions
wp-admin/edit.php?post_type=wp_navigation