Navigation: Use a single placeholder for the block #36023
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
tellthemachines
requested review from
talldan,
jasmussen,
noisysocks and
getdave
|
Size Change: +4.28 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
Fast work! Here's the one-step process selecting an existing menu: Here's creating a new one: In both cases, these take big steps towards managing the weight of the placeholder as it was before, nice work. There are still some larger issues to solve, but those are unrelated to this PR. A few small things we should fix, though: The footprint of the block is larger than the placeholder. This appears to be because the
This seems like it could be an existing issue, and this bit of CSS appears to fix it: CC: @vcanales for good measure. I also wonder if we shouldn't revisit the decision to make blue primary buttons. This screams a bit much with two buttons:
A small fix might be to just ensure there's only one primary blue button at a time. But I think it might be time to move towards this simpler layout: What do you think? That design takes small steps towards some of the ideas here. |
I forgot to respond to that one. Here's a mockup that adds separators and subheadings to the submenu: I don't think we necessarily need to get all the way to that design immediately — we can think of that more as a general pointer. But I do think separating things out could make sense. The verbiage is also up for grabs — I've currently used "Synced" as a term as inspired by a potential renaming of Reusable Blocks to Synced Blocks. We might even be fine with just calling it "Menus":
|
Hmm, that's weird. Here's what I'm seeing:
|
|
I added a CSS fix for that extra space and updated the dropdown to use separate MenuGroups with labels for Menus and Classic Menus. |
|
This is fast coming together, thank you for the work. The focus style trick appears to have worked: The submenu also has a nice subheading: Can you make the popover "isAlternate"? That will give it a dark border and appear to be more related to the in-canvas block UI. It's supposed to be enforced, but it doesn't appear to work here. I also still see those two blue buttons. Not sure exactly why. |
|
The odd positioning of the popover is tracked somewhere in the archive — it's due to the positioning math of the dropdown calculating based on the center of its opening button, rather than from the left edge. |
vcanales
reviewed
Oct 29, 2021
There was a problem hiding this comment.
The footprint of the block is larger than the placeholder. This appears to be because the gap property is spacing out the placeholder with a separate div that sits below it.
Regarding this, not rendering ResponsiveWrapper when the Placeholder is shown would be better than hiding it, ie. the following instead of a CSS rule:
{ ! isPlaceholderShown && (
<ResponsiveWrapper
id={ clientId }
onToggle={ setResponsiveMenuVisibility }
isOpen={ isResponsiveMenuOpen }
isResponsive={ 'never' !== overlayMenu }
isHiddenByDefault={ 'always' === overlayMenu }
>
{ isEntityAvailable && (
<NavigationInnerBlocks
isVisible={ ! isPlaceholderShown }
clientId={ clientId }
appender={ CustomAppender }
hasCustomPlaceholder={
!! CustomPlaceholder
}
orientation={ orientation }
/>
) }
</ResponsiveWrapper>
) }
I had neglected to update the logic that renders "Add all pages" as primary when the existing menus dropdown doesn't show 🤦 It should be fixed to take into account both classic and CPT menus now. I've also addressed the remaining feedback. |
talldan
reviewed
Nov 1, 2021
Comment on lines
144
to
148
| { isStillLoading && ( | ||
| <div> | ||
| <Spinner /> | ||
| </div> | ||
| ) } |
There was a problem hiding this comment.
Maybe this can be consolidated with the logic on line 138, so that there's only one loading state (<PlaceholderPreview isLoading />).
| <Spinner /> | ||
| </div> | ||
| ) } | ||
| { ! isStillLoading && ( |
There was a problem hiding this comment.
If the loading state is consolidated, it should be possible to move this up to line 139 to reduce the nesting.
Comment on lines
169
to
190
| <MenuItem | ||
| onClick={ () => { | ||
| setSelectedMenu( | ||
| menu.id | ||
| ); | ||
| onFinish( | ||
| menu | ||
| ); | ||
| } } | ||
| onClose={ | ||
| onClose | ||
| } | ||
| key={ | ||
| menu.id | ||
| } | ||
| > | ||
| { | ||
| menu | ||
| .title | ||
| .rendered | ||
| } | ||
| </MenuItem> |
There was a problem hiding this comment.
This code gets very deeply nested. It's very hard to read a side-by-side diff. I think it'd be good to consider breaking the placeholder down into separate components.
Or maybe just extract the dropdown menu for selecting an existing menu to a separate component.
| </div> | ||
| { hasMenus || canSwitchNavigationMenu ? ( | ||
| <DropdownMenu | ||
| text={ __( 'Add existing menu' ) } |
There was a problem hiding this comment.
I'm not sure 'Add' is the right word now that the wp_navigation menus are in this dropdown, since the user is directly editing the menu. Maybe 'Select existing menu'.
| </> | ||
| ) } | ||
| </DropdownMenu> | ||
| ) : undefined } |
There was a problem hiding this comment.
TIL that React now lets you render undefined.
|
Thanks for the review @talldan! I think I've addressed everything now. |
talldan
approved these changes
Nov 1, 2021
andrewserong
added
[Block] Navigation
andrewserong
changed the title
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.
Description
Closes #35818.
Goes back to the single-step placeholder for Navigation block. It's essentially the second step of the current 2-step flow, where:
One thing that might be confusing is that CPTs and classic menus are now indistinguishable from each other in the dropdown. It might be good to have two sections in there with some sort of heading before each. Ideas appreciated!
How has this been tested?
Add a Navigation block; try each of the three options that appear.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).