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

Experiment: Try stacked mover in block navigation #22314

Closed
wants to merge 3 commits into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented May 13, 2020

Description

Related #22089

This is not meant to be code reviewed or merged, but a quick experiment to see what stacked movers in the block navigation look like, and what they're like to use.

Some notes about this:

  • The button height is 18px, which is not quite as high as the stacked buttons on a block toolbar, which are 24px high. Each row would have to be made taller to achieve that height.
  • A really confusing aspect I found when testing is that using the arrow keys to navigate between the movers is unusual in this configuration. The issue is that you'd usually press the left or right arrow to change focus, but the buttons are visually vertical so it feels like up/down should be used instead. This also seems to be an issue with the main block toolbar now that arrow keys are used to navigate that.

How has this been tested?

Screenshots

Screenshot 2020-05-13 at 5 16 24 pm

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added the [Feature] List View Menu item in the top toolbar to select blocks from a list of links. label May 13, 2020
@talldan talldan self-assigned this May 13, 2020
@talldan talldan requested review from mapk and karmatosed May 13, 2020 09:32
@github-actions
Copy link

Size Change: +239 B (0%)

Total Size: 828 kB

Filename Size Change
build/block-editor/index.js 104 kB +19 B (0%)
build/block-editor/style-rtl.css 10.7 kB +109 B (1%)
build/block-editor/style.css 10.7 kB +111 B (1%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.59 kB 0 B
build/block-directory/style-rtl.css 764 B 0 B
build/block-directory/style.css 764 B 0 B
build/block-library/editor-rtl.css 7.12 kB 0 B
build/block-library/editor.css 7.12 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.38 kB 0 B
build/block-library/style.css 7.38 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 181 kB 0 B
build/components/style-rtl.css 17 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 5.59 kB 0 B
build/edit-navigation/style-rtl.css 618 B 0 B
build/edit-navigation/style.css 617 B 0 B
build/edit-post/index.js 28 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.1 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 8.37 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@karmatosed
Copy link
Member

karmatosed commented May 13, 2020

I'm not going to approve the review quite yet as there needs to be a decision here on movers that impacts wider than this issue. For those wanting to see what visually we have:

image

Now, this is just as per the iterations on #21935. However, I know that has a lot of debate to and fro. Ultimately, in our implementation here it's not about deciding how we have the movers as taking whatever decision comes from that issue into all movers. I don't see why these should differ. So, there needs to be a decision there we can then implement.

For me, I think using movers up and down makes sense, again I am on the side that wants a unified pattern here so bringing that in works. I do think behaviour wise there are a few things to iterate. Overall, this is working as I would expect which is great! Ideally, we shouldn't be able to have 2 movers showing at same time as in screenshot above.

To connect the conversation as moving should be a universe interaction, I am going to loop in @jasmussen here.

A really confusing aspect I found when testing is that using the arrow keys to navigate between the movers is unusual in this configuration. The issue is that you'd usually press the left or right arrow to change focus, but the buttons are visually vertical so it feels like up/down should be used instead. This also seems to be an issue with the main block toolbar now that arrow keys are used to navigate that.

This I am assuming is referring to keyboard navigation. It is a ponder and I wonder what we could do about implementing that @enriquesanchez?

@enriquesanchez
Copy link
Contributor

I agree the keyboard interaction feels a bit confusing, I was also expecting to use up/down to navigate between the movers.

One thing I'm not sure about is how this would be perceived by a visually impaired user. I'm assuming they won't have this same issue because they wouldn't perceive the stacked buttons, so they wouldn't have the expectation that they need to use the up/down arrows as I did. I'd like to think that for their use case, the current behavior makes more sense.

Again, the above is an assumption.

I also feel that the stacked movers have a very small hit target, which may make them hard to operate for folks with motor impairments. I assume the side by side movers have a bigger hit target?

What other benefits besides consistency with block movers are we gaining by having the movers stacked?

@mapk
Copy link
Contributor

mapk commented May 13, 2020

Ugh.. for some reason I'm getting this.

arrows

I'm going to step back, breathe, and assume I'm the problem here. 😄

The stacked arrows still cause me to do a double-take, and the limited click area feels tight. But I agree with Tammie, let's see what comes from the other issue to help inform the decision here.

@talldan
Copy link
Contributor Author

talldan commented May 14, 2020

Ideally, we shouldn't be able to have 2 movers showing at same time as in screenshot above.

@karmatosed I agree that doesn't look visually great. I'm not sure what can be done about that, the focused mover button has to be visible for keyboard users as does the hovered mover button for mouse users.

One thing I'm not sure about is how this would be perceived by a visually impaired user. I'm assuming they won't have this same issue because they wouldn't perceive the stacked buttons, so they wouldn't have the expectation that they need to use the up/down arrows as I did. I'd like to think that for their use case, the current behavior makes more sense.

@enriquesanchez I created a separate issue about the keyboard navigation - #22316, as this also happens on the block toolbar. I'm not sure how much of an issue it is based on what you mention above.

Ugh.. for some reason I'm getting this.

Wow, I couldn't reproduce it, but I have been noticing issues recently with Firefox maybe caching CSS, not sure if you're an FF user, @mapk.

@jasmussen
Copy link
Contributor

This is a really tricky design to get right, and lest we repeat the mistakes of movers in the editing canvas (additional tabstops) it's prudent to do this carefully.

Zooming out a little bit and looking for a generic solution, I notice the "Navigation structure" is present for the Navigation block, but not for either Buttons or Social Links which strucutrally are the same. Although not part of this PR, any efforts to improve this interface should consider not just those other blocks, but perhaps any block with nesting, and indeed the block navigation popover itself. This likely does not change the particular implementation, but important to keep in mind so we don't write software just for the Navigation block.

With regards to the specific implementation — it seems like there's an opportunity here to align with the efforts for improving the block mover for blocks. Specifically this design which I strongly feel we should try as soon as we can because it's so simple seems like it could work here as well — there's already a block icon. What if movers were integrated next to that, as a callback to where they will be on blocks?

We could also be consistent with blocks in only showing the mover control onselect — this would reduce flickering and also eliminate tabstops.

Seems like a mockup/prototyping effort exploring that might be an interesting next step.

@talldan
Copy link
Contributor Author

talldan commented May 21, 2020

Hey @jasmussen, thanks for taking a look. Sorry it took a little while to respond.

Although not part of this PR, any efforts to improve this interface should consider not just those other blocks, but perhaps any block with nesting, and indeed the block navigation popover itself. This likely does not change the particular implementation, but important to keep in mind so we don't write software just for the Navigation block.

Oh definitely. The iteration is mainly happening on the navigation block as part of an effort to improve the Navigation Menu page (because it really needs these features), and also because that block is experimental so we have more ability to break things 😄

It'll be easy to switch these features on in other places when the time comes.

We could also be consistent with blocks in only showing the mover control onselect — this would reduce flickering and also eliminate tabstops.

The issue here is that there isn't really persistent selection in the block navigation. Clicking a block typically transfers focus to the block itself, and in the post editor it actually closes the popover.

Potentially that could change though, I think it's a difficult design to get right as this tool becomes more about editing rather than just selecting, we're trying to fit a lot into a small space.

@jasmussen
Copy link
Contributor

It'll be easy to switch these features on in other places when the time comes.

💯

Glad to hear that you're learning in code, it definitely helps inform things. Just important that we silo it to just one block!

@mapk
Copy link
Contributor

mapk commented May 26, 2020

With regards to the specific implementation — it seems like there's an opportunity here to align with the efforts for improving the block mover for blocks. Specifically this design which I strongly feel we should try as soon as we can because it's so simple seems like it could work here as well — there's already a block icon. What if movers were integrated next to that, as a callback to where they will be on blocks?

We really need a decision on that movers issue. It's beginning to hold up other explorations. 😬

In response to displaying the stacked movers next to the icons, one of the better design iterations is removing the icons from the Navigator. (#22497) This lightens the UI and saves much needed horizontal space.


In reference to the stacked movers, here's how they worked for me.

movers

Feedback

  1. There appeared to be a bug where after clicking the up mover, then immediately clicking the down mover didn't do anything. You can see it happen several times in the gif above.
  2. The stacked arrows appeared quite overwhelming in this tight space, especially while hovering through the items.
  3. Due to the imperfect nature of close proximity hover states, my cursor easily ended up in situations that left me more confused where I saw multiple arrows displayed.

Screen Shot 2020-05-26 at 4 44 54 PM

If we do take a stacked mover approach, it's becoming more evident to me that we'll need to use the smaller arrows instead of these larger chevrons. (#21935 (comment)) unless we don't mind changing them out for the smaller arrows in the tighter places like this Navigator.

@jasmussen
Copy link
Contributor

We really need a decision on that movers issue.

It needs a PR more than anything — here's one: #22673 🎉

If we do take a stacked mover approach, it's becoming more evident to me that we'll need to use the smaller arrows instead of these larger chevrons.

I'm not sure — I feel like the design Tammie and I landed on in #22497 (comment) strikes a good balance!

@ZebulanStanphill
Copy link
Member

Yeah, if the movers appear too squished, then just increase the height of the items in the block navigator to be the same height as the block toolbars.

@mapk
Copy link
Contributor

mapk commented May 28, 2020

I feel like the design Tammie and I landed on in #22497 (comment) strikes a good balance!

I left a comment there, but they still feel visually imbalanced. @ZebulanStanphill brings up an interesting solution... if we really want to keep the larger chevrons, maybe we need to widen the item.

@jasmussen
Copy link
Contributor

jasmussen commented May 29, 2020

I left a comment there, but they still feel visually imbalanced. @ZebulanStanphill brings up an interesting solution... if we really want to keep the larger chevrons, maybe we need to widen the item.

Great call, both. I actually also responded in #22497 (comment) addressing this. The line items are 48px tall affording two 24x24 (minimum touch size) targets for the movers, but they have been balanced better to match the new mover control.

Thanks for your patience!

@talldan
Copy link
Contributor Author

talldan commented Jun 11, 2020

Superseded by #22796

@talldan talldan closed this Jun 11, 2020
@talldan talldan deleted the try/stacked-mover-in-block-navigation branch June 11, 2020 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants