Update navigation editor placeholder #34568
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
talldan
added
[Type] Enhancement
talldan
requested review from
draganescu,
getdave,
kevin940726 and
tellthemachines
as code owners
|
Size Change: +2.4 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
Thanks for working on this, @talldan. It looks much better from a UX perspective, in my opiniton. If the current goal is to achieve parity with the "old" navigation screen, I think we can remove the "Copy existing menu" option. It doesn't exist on the current screen. Minor code related notes:
|
Mamaduka
reviewed
Sep 6, 2021
packages/edit-navigation/src/components/block-placeholder/index.js
Outdated
Show resolved
Hide resolved
Mamaduka
reviewed
Sep 6, 2021
packages/edit-navigation/src/components/block-placeholder/menu-items-to-blocks.js
Outdated
Show resolved
Hide resolved
|
I think this also fixes #30745. |
|
This looks nice. I would improve a couple of things:
|
If we were to do that and if we also had the interface to add links in bulk… we wouldn't need this placeholder state, right? I think that would simplify things a lot. |
In that case, I think we can just display the "blank state" when a new menu is created. We'll need to solve #31276 as well. |
|
I think copy existing menu is a nice feature to have. That is the user me who constantly fiddles with his WordPress blog. |
I think it's a good feature too, but maybe we could have a "Duplicate this menu" option or a way to paste a copied navigation instead (which I think we don't have, right?), and remove one step from the menu creation process. |
Co-authored-by: George Mamadashvili <[email protected]>
talldan
force-pushed
the
update/navigation-editor-placeholder
branch
from
17a6ac6
to
b386aba
Compare
|
I think the copy menu option is a good feature, but then I like the duplicate menu idea too. I also quite like that the navigation editor guides the user through creating a menu in a very friendly way after this change, I wouldn't feel so good about leaving the user with an empty block after a menu has been created. My opinion is that it could be revisited, but right now the placeholder that's in |
|
I believe I've address most of the feedback now. There are a couple of things I haven't been able to address.
The link style buttons do have a focus and hover styles, so I'm not sure it'd be good to reduce this to absolute zero. I've reduced it to 4px so that there's only a little gap between those borders:
Similar to the above, I don't think the component library has a true link style button (I believe the accessibility team have rejected that as being too confusing in the past), so there are still hover/focus borders to take into account with the positioning: |
Mamaduka
reviewed
Sep 7, 2021
packages/edit-navigation/src/components/block-placeholder/index.js
Outdated
Show resolved
Hide resolved
Mamaduka
reviewed
Sep 7, 2021
packages/edit-navigation/src/components/block-placeholder/index.js
Outdated
Show resolved
Hide resolved
|
What if we make the "Start blank" button a "primary" button? This is how the "Template Parts" block does it. |
|
Thanks for the re-review @Mamaduka, I think I've addressed all of that.
Here's an example of how that would look: I like it. What do you think @javierarce? I suppose it's a matter of whether we want to encourage users down the 'Start blank' path. |
|
Cool, lets merge, and then if we want to try any other options discussed here it should be easy to iterate. |
This was referenced Sep 8, 2021
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
Fixes #33999
Fixes #23953
Fixes #30745
Updates the navigation editor's block placeholder when creating a new navigation block.
Implementation wise, this doesn't feel ideal. There was a lot of discussion on #23953 about a possible implementation that didn't affect the navigation block. When I started looking at this, I discovered the no matter what we do, the navigation block's current placeholder 'preview' needed to be disabled somehow as it always showed on block deselection:
I found it was equally as simple to disable this via a filter as it was to pass a replacement placeholder component into the block. So this is what I chose as the simplest approach.
Ultimately, this is an important piece for the navigation editor, and I think it's good to prioritise this as much as possible.
A problem with this change is that this now exposes the issue described in #31276 even more after clicking 'Start blank' and deselecting the block, so I'll start looking at that next.
How has this been tested?
It has existing e2e tests that have been updated.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).