Split useNavigationMenu into bite-size functions and add unit tests#41139
Merged
Split useNavigationMenu into bite-size functions and add unit tests#41139
Conversation
|
Size Change: +2.17 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
getdave
reviewed
May 20, 2022
Contributor
There was a problem hiding this comment.
Thanks for the great refactor and the test coverage 👏
My only slight concern is the API itself useNavigationMenu(). Despite the singular terminology, this actually returns information about all menus as well.
Would this be expected as a consumer of the hook? For example:
useNavigationMenu(2)...will give me information about all menus as well as menu 2.
Alternatively we could split this into x2 hooks - one for fetching all menus and the other for a single menu?
Also left some questions and nits.
packages/block-library/src/navigation/test/use-navigation-menu.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/navigation/test/use-navigation-menu.js
Outdated
Show resolved
Hide resolved
getdave
reviewed
May 20, 2022
Contributor
There was a problem hiding this comment.
I also just noticed the test failure on
shows a loading indicator whilst ref resolves to Navigation post items
This is supposed to check that a loading indicator shows up whilst the Navigation menu is being requested.
It makes me slightly nervous given how much refactoring has happened in this PR. That said, when I tested manually the functionality seemed to work.
Screen.Capture.on.2022-05-20.at.10-34-11.mp4
Might be worth running that test locally in "non-headless" mode and observing the results.
Contributor
Author
@getdave I do agree the naming is unfortunate. Still, this is the name we use today so I'd keep the renaming outside of scope of this PR. I'm happy to follow up with another PR. |
352f73f to
d06adb5
Compare
Contributor
Author
|
@getdave I've got that e2e test to pass locally, let's wait for the CI now:
|
Contributor
|
In the trunk version I liked that the return values' names were easier to grok I'd not shy away from listing the member names of the return object and destructuring them from each select* before return in the hook. |
Contributor
Author
|
@draganescu done in 8001e79 |
draganescu
approved these changes
Jun 13, 2022
Contributor
draganescu
left a comment
There was a problem hiding this comment.
If the tests pass the rest LGTM 👍🏻
getdave
approved these changes
Jun 13, 2022
Contributor
getdave
left a comment
There was a problem hiding this comment.
This is a solid refactor. Thank you for fixes the feedback I left before. LGTM 👏
packages/block-library/src/navigation/test/use-navigation-menu.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/navigation/test/use-navigation-menu.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
…ng them from each select
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
5daa673 to
a7d7540
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
The
useNavigationMenuhook got quite long and complex with its numerous ternary operators. This PR means to make it easier to reason about by splitting it into smaller pieces and adding unit tests .Testing Instructions
cc @getdave @talldan @scruffian @draganescu @noisysocks