Remove custom button and (conditionally) show single menu on Navigation route in Browse Mode #51565
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
added
[Type] Code Quality
|
Size Change: -531 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in edb424f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5309640103
|
This comment was marked as resolved.
This comment was marked as resolved.
getdave
force-pushed
the
fix/navigation-listing-screen-conditional-rendering
branch
from
a176086 to
3dee12c
Compare
getdave
requested review from
youknowriad,
draganescu,
scruffian,
jeryj,
MaggieCabrera and
ajlende
|
This PR does expose some of the loading inadequacies. Let's tidy those in a follow up. |
getdave
commented
Jun 16, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menus/index.js
Outdated
Show resolved
Hide resolved
This was referenced Jun 19, 2023
This comment was marked as resolved.
This comment was marked as resolved.
scruffian
reviewed
Jun 20, 2023
| ); | ||
| } | ||
| }; | ||
| const _handleDelete = () => handleDelete( navigationMenu ); |
There was a problem hiding this comment.
Is this a pattern we use elsewhere? Another option could be to call it handleDeleteBound...
There was a problem hiding this comment.
Yeh I mean...it isn't exactly elegant 🤷
I'm not 100% about the API on this hook yet. Let's allow new requirements to drive any further refactoring however.
scruffian
deleted the
fix/navigation-listing-screen-conditional-rendering
branch
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…on route in Browse Mode (WordPress#51565) * Remove custom button and show single menu on Navigation route * Extract hooks * Pass menu to all hooks * Use SingleMenu component and hooks on Navigation listing route * Move hooks to file file * Consolidate to a single hook * Improve hooks by passing arg at call time * Rename hooks
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?
Renders a single Navigation directly on the
/navigationroute of the Site Editor if there is only a single menu. If there are multiple menus then we render a list as pertrunk.Closes #51561
Why?
Reasoning was outlined by Riad in #51561.
This change avoids requiring consumers of the Site Editor routes from having to programmatically determine which route to send someone to if they have a single menu vs multiple menus.
We encapsulate the behaviour and logic inside the route components and thus a consumer can simply go to
/navigationand know they will see the "right" thing.How?
Extracts the code for displaying a single menu into reusable component and hooks. Then shares this between the routes for both
/navigationrouteTesting Instructions
Navigationin Browse Mode.Navigation.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-06-16.at.15-47-10.mp4