-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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
@getdave I've got that e2e test to pass locally, let's wait for the CI now:
|
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. |
@draganescu done in 8001e79 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tests pass the rest LGTM 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
What?
The
useNavigationMenu
hook 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