Skip to content
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

Navigation Screen: Design Iteration #28675

Merged
merged 21 commits into from
Feb 8, 2021
Merged

Navigation Screen: Design Iteration #28675

merged 21 commits into from
Feb 8, 2021

Conversation

shaunandrews
Copy link
Contributor

@shaunandrews shaunandrews commented Feb 2, 2021

The current design of the beta Navigation screen looks like this:

image

There's a few problems here:

  • The large white canvas implies that you can/should fill it with blocks. However, most menus are likely to contain only a handful of links.
  • The ever-present List View is really just a duplicate view of the currently selected menu. It doesn't offer much, and I can't justify the duplication of elements.
  • The Navigation block defaults to it's horizontal variation, which doesn't feel familiar when coming from the existing menus screen. Related, the way submenu items appear/disappear makes it difficult to manage them.
  • The inspector sidebar is hidden inside a popover. The location of the cog icon moves depending on the block selected, making it hard to find as you add and move blocks.

This PR aims to resolve those problems, and give the beta Navigation screen solid footing for future improvements. Here's the updated design as seen in this PR:

image

  • The refreshed screen ditches the all-white canvas, focusing instead on the singular Navigation block on a familiar gray, wp-admin background.
  • The inspector appears as a persistent sidebar on the right, offering quick access to menu and link settings.
  • I've adjusted the layout and increased the spacing of the Navigation block and it's inner Link blocks to make them easier to select.
  • Submenu icons now appear to the left of the label.
  • Submenus no longer appear as a floating box; Instead they appear indented in the normal flow of the page.
  • Links blocks now show a light gray border when hovered.
  • The focus outline now appears when editing a Link's label.
  • The toolbar is no longer in fixedToolbar mode.
  • When editing a submenu, the parent link gets a border.

@shaunandrews shaunandrews added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Feature] Navigation Screen labels Feb 2, 2021
@annezazu
Copy link
Contributor

annezazu commented Feb 3, 2021

This looks great! The only thing that stood out to me to create more consistency between widgets and navigation screen is the overall top toolbar. Here's the current design for Widgets:

Screen Shot 2021-02-02 at 5 27 39 PM

Right now, the design above makes that top toolbar to add new, manage locations, etc feel like it's floating/disconnected from the experience:

Screen Shot 2021-02-02 at 5 34 42 PM

I don't know if more major design changes are expected for widgets but felt worth noting. Finally, for the "Editing: blog footer" section, this feels a bit awkward to me for some reason. I almost wonder if this could take a page out of the template navigation experience? This part specifically:

Screen Shot 2021-02-02 at 5 43 14 PM

Here's a pretend "mockup" if it can even be called that:

Screen Shot 2021-02-02 at 5 44 18 PM

@jasmussen
Copy link
Contributor

I love the track this is on.

Whether or not to top toolbar is a conversation worth having — I lean on the side of not replicating the block editor because these are screens built for different purposes, even if they are still technically block editors. However I also trust you and the others to consider all the challenges involved in choosing which direction to go.

When I spin up this PR I see this:

Screenshot 2021-02-03 at 09 35 34

  • The "No menus available" would likely benefit, separately, from that separate mockup you made of either creating one automatically, or making it the first choice you make when visiting this screen.
  • You can probably hide the footer.
  • Same with "You are using an experimental full site editing theme". In fact you can/should probably hide every core notice on this screen, just as the block editor does.
  • Top toolbar or not, the "Navigation" heading could benefit from the same size as the other screens.

Screenshot 2021-02-03 at 09 40 19

There's a lot of space under each menu item. That's from an empty submenu that has a min height of 44px. So there's a little juggling to do there.

Almost certainly a pre-existing and separate issue, but reloading the screen I got this:

Screenshot 2021-02-03 at 09 43 05

I like the idea generally of forcing the menu to be vertical. But it may end up being more work than is good. Just one example:

Screenshot 2021-02-03 at 09 45 14

The mover assumes a horizontal menu. There are probably going to be more issues as well. It could still be worth it, but it might be best to do the vertical menu as a separte PR exploration, so as to not hold up this one.

Also most probably separate, the sidebar menu could use a little love. Delete is centered and the prase above with the checkbox wraps.

Screenshot 2021-02-03 at 09 40 07

@jasmussen
Copy link
Contributor

I forgot to mention: if you like, I'm happy to set aside some time to collaborate with you on this PR with you. Just let me know!

@jasmussen
Copy link
Contributor

To expand on forcing vertical navigation, the addition of the permanently expanded list view was probably created to address that very same issue: making it easier to configure a menu that has more than a couple of nested levels.

Because we don't have a solid alternative, and to keep the good things from this PR, perhaps it's best to keep that list view for now and explore how to better integrate that separately.

@draganescu
Copy link
Contributor

A few things in my mind:

  • If we force the vertical layout, why are we not using the block list view (block navigator component) and instead default to the block? Isn't that component better fit, especially for heavy nesting?
    • The main idea here is heavy nesting. These menus can get quite heavy. The list view will have (at some point) the option to "drill-down", but the block is likely to display everything, which will make small fixes and updates pretty cumbersome.
  • I would like to add my "vote" FOR keeping the top toolbar and allowing us to use the interface package. It is only worthwile to create new components if the reasoning is giving strong benefits and it's not, in my opinion in this case.
  • I need to be able to delete a menu
  • The list of possible actions in the toolbar, to me, seem lumped together, there is no sense of importance. What bothers me in particular is the "add new" action. It is the only way to add a menu, it can't be just a link button like that, in the middle of other things.
  • Some items will probably have to be drop-downs (select menu, choose location).
  • As a label, "select menu" might work better as "switch menu" since you're always landing in one menu, there is no previous selection step. I would also attempt to "connect it" to the current edited menu's name, but that may be wrong.
  • In this new editor, the meaning of manage locations is broken. We should reconsider thins, possibly remove the idea and instead go forward with "Assign menu to location" or similar, as an Inspector property. The reason why I believe this is because in this version of the UI it is an important action that is done in a small square in an unrelated page (I can assign menu A while editing B, which is dangerous).

@mtias
Copy link
Member

mtias commented Feb 3, 2021

If we force the vertical layout, why are we not using the block list view (block navigator component) and instead default to the block? Isn't that component better fit, especially for heavy nesting?

It's not the best UI for just a handful of links (most navigations?). It might be good to switch or default to list view open if we detect more than one or two levels of nesting.

@talldan
Copy link
Contributor

talldan commented Feb 3, 2021

I need to be able to delete a menu
The list of possible actions in the toolbar, to me, seem lumped together, there is no sense of importance. What bothers me in particular is the "add new" action. It is the only way to add a menu, it can't be just a link button like that, in the middle of other things.
Some items will probably have to be drop-downs (select menu, choose location).

@draganescu These things are actually the same as they are in master. Still ok as feedback, but just thought I'd mention that they're unrelated to the changes in the PR. The delete menu button is in the navigation block's block inspector. I made an issue suggesting we revisit this - #28184.

I'm starting to wonder if we should ditch the nav block and just treat this as a normal block list with the same blocks that the nav block supports. The nav block's settings could be the equivalent of document settings. Not really sure the nav block is offering much apart from 'Show submenu indicators'.

@talldan
Copy link
Contributor

talldan commented Feb 3, 2021

Separately I made some PRs that could be merged into this.

@shaunandrews
Copy link
Contributor Author

I'm starting to wonder if we should ditch the nav block and just treat this as a normal block list with the same blocks that the nav block supports.

A similar thought occurred to me while working on this PR; Adding a Menu block that is container meant specifically for the context of this dedicated screen. I'm not sure what the pros/cons are if we diverge from the Navigation block, so I mostly avoided the thought.

@jasmussen
Copy link
Contributor

A similar thought occurred to me while working on this PR; Adding a Menu block that is container meant specifically for the context of this dedicated screen. I'm not sure what the pros/cons are if we diverge from the Navigation block, so I mostly avoided the thought.

Ultimately I'll defer to you all on timing, complexity, experience. My instinct is that there's value in some wysiwyg even on this screen.

@talldan
Copy link
Contributor

talldan commented Feb 5, 2021

#28769 switches from a fixed toolbar to a floating one as discussed in #28689.

@shaunandrews
Copy link
Contributor Author

I think this one is is a good place to merge.

There is plenty more work to do, but this is an experimental, beta screen. I feel pretty comfortable with merging this as-is, and continuing to iterate.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few issues remaining, but given how big this PR is I'm going to approve and merge, as it'll be easier to tackle them in smaller parallel PRs.

Things that seem like they'll be good follow-ups:

  • Work on responsive designs/styles for the new screen
  • Reimplement Clear block selection in the navigation editor when clicking editor canvas #28382, which regressed in this PR.
  • Improve the way this works with the Page List block, which was merged just before this PR. Page List still shows flyout style menus.
  • Accessibility testing
  • Find a solution to the amount of extra navigation block styling this introduces (mentioned in comment below)

Thanks for all your work @shaunandrews, the new design is an exciting step!

Comment on lines 14 to 96
.wp-block-navigation {
margin: 0;
font-size: 15px;
padding: $grid-unit-15;
}

.components-spinner {
.wp-block-navigation-link {
display: block;
margin: 10px auto;

// The following attempts to fix the focus outlines
&.is-selected > .wp-block-navigation-link__content,
&.is-selected:hover > .wp-block-navigation-link__content {
box-shadow: 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
}

&.block-editor-block-list__block:not([contenteditable]):focus::after {
display: none;
}
}
}

.edit-navigation-editor__list-view {
border-top: 1px solid $gray-300;
padding: $grid-unit-20;
.wp-block-navigation-link__content {
padding: 0;
margin-bottom: 6px;//$grid-unit-05;
border-radius: $radius-block-ui;

@include break-medium() {
border-left: 1px solid $gray-300;
border-top: none;
margin-top: 0;
width: 300px;
&:hover {
box-shadow: 0 0 0 $border-width $gray-300;
}
}

.edit-navigation-editor__list-view-title {
font-size: 1.2em;
margin: 0;
.wp-block-navigation-link__label {
padding: $grid-unit-15;
padding-left: $grid-unit-30;

// Without this Links with submenus display a pointer.
cursor: text;
}

.components-spinner {
display: block;
margin: 100px auto;
// Position the submenu icon so it appears to the left of
// the Link. All the extra specificity is help override the
// rotation on the SVG.
.wp-block-navigation .wp-block-navigation-link .wp-block-navigation-link__submenu-icon {
position: absolute;
top: 6px;
left: 2px;
padding: 6px;
pointer-events: none;

svg {
padding: 0;
transform: rotate(90deg);
}
}

// Submenus
// There's a bunch of stuff to override just to get submenus to
// as a normal block element.
.wp-block-navigation-link.has-child {
cursor: default;
border-radius: $radius-block-ui;
}

// When editing a link with children, highlight the parent
// and adjust the spacing and submenu icon.
.wp-block-navigation-link.has-child.is-editing {
> .wp-block-navigation__container {
opacity: 1;
visibility: visible;
position: relative;
background: transparent;
top: auto;
left: auto;
padding-left: $grid-unit-15;
min-width: auto;
width: 100%;
border: none;
display: block;

&::before {
display: none;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maintainability of all the block styles being defined here will be difficult. If the block's own styles change there's the strong potential for bugs in the navigation editor. That or unused styles. The goal seems to be to make the submenu items appear inline (not as a flyout menu):
Screenshot 2021-02-08 at 2 00 38 pm

As a follow-up it'll be worth thinking about how we can solve this. For example, perhaps this is a style variation of the nav block?

We'll also have to make this work for the Page List block which was just merged to master.

Would be interested in your thoughts @tellthemachines.

Copy link
Contributor

@talldan talldan Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I remember now about the discussion around using a dropdown block for submenus (#23745), so maybe in some distant future the nav screen would use a collapse/expand block (I completely forget the proper name for this?) instead of dropdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants