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

Try a new hover and select approach to improve nested block selection #6773

Merged
merged 14 commits into from Jun 1, 2018

Conversation

@jasmussen
Contributor

jasmussen commented May 16, 2018

This PR aims to solve a number of issues we've had around selecting nested and parent blocks. Watch the GIF first:

nested blocks

It also tries to take the vinegar out of the hover label, and makes it feel much less fiddly to work with blocks in nested situations.

It does this by making the parent block side padding much wider than the nested block padding. But only in a visual way, using negative margins on the UI. That means the appearance of the blocks should not change visually, the goal is still to be 1:1 with the front-end, and further enhancements to nested block paddings are coming in #6408.

For starters, please try out this branch and get a feel for it. Depending on feedback and aside from polish, there are two remaining to-do's for this branch:

  1. I'd like help with the mobile solution, which is that you click a block with children and select the parent first, and then click again to select or set the caret in a child block.
  2. We should look at tuning the keyboard navigation so when you arrow-key down from a paragraph and into a block that has children, you first select the block itself, and then navigate into the child blocks.

Your thoughts and feedback are appreciated.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 16, 2018

This has been rebased and squashed, so the quote block no longer uses nested blocks like it does in the GIF above. Please test using the Columns block.

@jasmussen jasmussen requested review from karmatosed and mtias May 16, 2018

@afercia

@jasmussen thanks for the ping. Tested with columns and two quotes. First impression is that is not immediately clear when/why the "breadcrumb" labels appear and when not. They appear on hover, but not when the block is selected, that's a bit confusing.

Aside: I guess the breadcrumb labels were introduced specifically for the nested block but they also reveal that sometimes there's the need to identify what a block is. They're basically visible labels, I wonder if it could be beneficial to always show them, at least when the block is selected.

The color contrast of the breadcrumb label is not sufficient. See .editor-block-list__breadcrumb has two background CSS properties, the latter prevails and it's $blue-medium-300 #66C6E4 with white text. The contrast ratio of 1.95:1 https://jdlsn.com/color/?type=hex&color=FFFFFF&color2=66C6E4 should be at least 4.5:1

screen shot 2018-05-16 at 11 47 58

In some cases, for example when hovering the "horizontal inserter" the label appears:

horiontal inserter nested blocks

That's potentially very confusing, as users may think the label is related to the inserter.

Keyboard interaction: yes as you pointed out there's the need to improve it. Also, the "Add block" plus icon appear only on focus, which makes them hardly discoverable.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 16, 2018

Great review, will try to address your points. Keep it coming.

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented May 16, 2018

I think this is a big step in making nested blocks easier to work with.

See also #6224, which isn't explicitly related, but definitely has some crossover in terms of exploring the block interaction UI.

@karmatosed

This is a huge improvement! Thanks for working on this @jasmussen. Experience and design wise I am going to approve this, noting that the accessibility improvements are already being worked on.

@mtias mtias added this to the 3.0 milestone May 23, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 24, 2018

Rebased, squashed, and fixed an issue with full-wide blocks.

@@ -647,6 +653,9 @@ const applyWithSelect = withSelect( ( select, { uid, rootUID } ) => {
block,
isSelected,
hasFixedToolbar,
rootUIDOfRoot: getBlockRootUID( rootUID ),
orderOfRoot: getBlockIndex( rootUID, getBlockRootUID( rootUID ) ),

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Unused?

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Unused?

Removed in 11d7161.

@@ -647,6 +653,9 @@ const applyWithSelect = withSelect( ( select, { uid, rootUID } ) => {
block,
isSelected,
hasFixedToolbar,
rootUIDOfRoot: getBlockRootUID( rootUID ),

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Unused?

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Unused?

Removed in 11d7161.

@@ -647,6 +653,9 @@ const applyWithSelect = withSelect( ( select, { uid, rootUID } ) => {
block,
isSelected,
hasFixedToolbar,
rootUIDOfRoot: getBlockRootUID( rootUID ),
orderOfRoot: getBlockIndex( rootUID, getBlockRootUID( rootUID ) ),
isSelected,

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Duplicated (already assigned above). Thinking there ought to be an ESLint rule for this.

Edit: There is and it's enabled. I'm guessing it doesn't support the ES2015 property shorthand in the version we're using.

Edit 2: It is actually error-ing 😄

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Duplicated (already assigned above).

Removed in 11d7161.

) }
{ rootUID && ( <span> &rarr; </span> ) }

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Directionality has me wondering how this appears on RTL.

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Is the span necessary? Could this be { rootUID && ' → ' } ?

Edit: I see we're applying styles. We should probably do so with a class.

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Directionality has me wondering how this appears on RTL.

[...]

Is the span necessary? Could this be { rootUID && ' → ' } ?

Updated to an element with :after pseudo-class content (and RTL flipping) in e650954.

@@ -53,7 +52,7 @@ export class BlockBreadcrumb extends Component {
}
render( ) {
const { uid, rootUID, selectRootBlock, isHidden } = this.props;

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

If we're not using selectRootBlock, the composed withDispatch can be removed.

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

If we're not using selectRootBlock, the composed withDispatch can be removed.

Removed in 11d7161.

*
* @return {boolean} Whether the block as an inner block selected
*/
export function hasSelectedInnerBlock( state, uid ) {

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

What about an inner multi-selection?

@aduth

This comment has been minimized.

Member

aduth commented May 24, 2018

3a0aac3 changes the behavior to tab to the block focus stop first if there are inner children, before proceeding to the inner blocks (in order, so ArrowDown from a previous block).

b2c869a adds an overlay on small screen unselected root blocks so that it's required to tap on a nested block wrapper before tapping one of its child blocks.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 25, 2018

Thanks for those two fixes, @aduth, it's working great!

I noticed that I can't add anything to the 2nd column in the columns block:

columns

Do you know what might have caused that?

@mtias

This comment has been minimized.

Contributor

mtias commented May 26, 2018

Looking good! Some thing to address:

  • Wide blocks look broken.
  • The inner paddings on left and right look a bit large to me.
  • With these outlines I have the urge to remove the gray ones when you click on a block. What do you think?
  • Sibling inserter is getting too much in the way — can't click the label.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 28, 2018

Pushed a few fixes:

  • The columns block was broken in a rebase. Fixed that. The appender now works in the right column.
  • Made the breadcrumb sit inside, and fade same as the outlines.

new

With these outlines I have the urge to remove the gray ones when you click on a block. What do you think?

I worry that will make it a bit fiddly to know what you're interacting with. The biggest challenge is that this will make the block toolbar sit in a fashion that might make it look detached.

We can definitely explore addressing that — perhaps the block toolbar gets a little triangle arrow that shows what it's connected to. But I think this should be explored separately.

Sibling inserter is getting too much in the way — can't click the label.

The sibling inserter does need a little further love since the most recent iteration, but at least now you don't accidentally invoke it all the time. But can you clarify why you'd want to click the label? The label doesn't do anything anymore, it's purely informative.

The inner paddings on left and right look a bit large to me.

I know. This is because we reserve 28px to the left of all blocks for the side UI, so the clickable area to select the parent actually starts outside of that clickable area:

wide paddings

Right now the extra wide padding is present on all top level blocks, but not on nested blocks. Some options:

  • Make the side UI less wide. Not ideal.
  • Try to increase the padding above and below to make it seem more "even"/symmetrical all around.
  • Make the wide paddings apply only to blocks that have inner blocks. Paragraphs would look like they do in master, but columns, and in the future other blocks that have children, would get the wide paragraphs.
  • Right now the area below the side UI is used for the draggable area for dragondrop. This extends the height of the block. We could make it so it's really just the up/down arrow that double as the drag handle — then you'd be able to click below the up/down arrows to select the parent block.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 28, 2018

P.S. I could use help with a rebase, not sure what's up with the store now.

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented May 28, 2018

@jasmussen I think making the up/down arrows double as drag handles is a great idea. I think that if you did that, you could also make the block toolbar double as a drag handle as well, providing more than enough space to drag a block from without adding any additional weight to the UI.

Right now, I think the dragging from the side of blocks is kind of confusing. It looks like you are outside the block boundaries, and there is little indicator that the side area is connected to the block. This could be resolved with adding a hover effect to the side of blocks, but I would prefer if the side padding was simply done away with and the arrow buttons and/or block toolbar were made into drag handles.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 29, 2018

Quick feedback:

  • Can we drop the line from the in-between inserter, I feel the "plus" button in the middle is enough?
  • The post title width (with outline) is different than the blocks (same for the code editor but this may be a separate issue, I think if we switch the editor mode, things should stay in their exact position)
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 29, 2018

Good feedback, thank you, and thanks for the rebase help!

Can we drop the line from the in-between inserter, I feel the "plus" button in the middle is enough?

I agree the sibling inserter needs a little love. The plus in the middle is the only clickable element, which helped make it somewhat more intentional to access, but this isn't working when you are trying to insert above the block you have selected, because the plus sits behind the block toolbar.

The line indicates where the block will be inserted — perhaps we could color it according to the theme to make it more clear what it means, but I'm not sure if removing it is the solution.

Does it make sense to simply not be able to use the sibling inserter before or after the block that is selected? Does it make sense to require that you simply have no block selected in order to use the sibling inserter? I feel like the answer to both of those are "no", but I'm not sure how to reconcile it otherwise.

Another solution is to show the plus to the left or right of the sibling inserter, but I tried this in the branch and it conflicted with the side UI. Perhaps we can show it waaay left or waaay right of the block, to not conflict?

The post title width (with outline) is different than the blocks (same for the code editor but this may be a separate issue, I think if we switch the editor mode, things should stay in their exact position)

This should be fixed in #6909

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 29, 2018

I think it's already too crowded to add a line on hover. Maybe the line could only be shown when we hover the button directly. Just my opinion. The issue for me is not related to the selected block.

Also, I personally don't like the "blue" color of the hover outlines, I know it's done on purpose but it feels odd with the rest of the UI

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 29, 2018

I noticed a small issue when you hover empty paragraph blocks, the inserter on the left should have a white background to hide the outline or something.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 30, 2018

Okay, from an experience and CSS point of view, this is now ready.

This PR brings a new way of selecting blocks, including parent blocks, both for mouse, keyboard and mobile users. It refactors a lot of variables and refines a bunch of styles in order to get there, and the hover label has been re-thoughts.

There's one last thing that I'd like help with, @gziolo or @youknowriad or @jorgefilipecosta if you have time — and that is hiding the hover outline and hover label from an empty paragraph block:

screen shot 2018-05-30 at 13 52 18

Other than that, I think it's ready for a final review.

@notnownikki

This comment has been minimized.

Member

notnownikki commented May 30, 2018

Just one little thing I found, on a classic block the hover label is obscured by the classic block header.

overlap

Also, I feel like the blue doesn't quite fit in with the color scheme... but no one ever asked me to coordinate their outfit colors, so perhaps don't pay too much attention to that :)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 30, 2018

Great catches. Will fix the classic block, it looks wrong entirely, and using a theme color for hover and label might be a fun splash of color.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 30, 2018

Changing the color of the hovered block and the label to match the admin theme is easy to do, but I would like to do it separately to keep this PR from growing further.

I fixed the classic block:

screen shot 2018-05-30 at 16 33 44

@notnownikki

This comment has been minimized.

Member

notnownikki commented May 30, 2018

Looking good!

@mtias

This comment has been minimized.

Contributor

mtias commented May 30, 2018

I'm seeing the spacer block missing controls on this branch:

image

@aduth

This comment has been minimized.

Member

aduth commented May 30, 2018

I'm seeing the spacer block missing controls on this branch:

Noting as unrelated to these changes. See #7024, #6922 (comment).

@aduth

aduth approved these changes May 31, 2018

Code-wise 👍

&:hover,
&:active,
&:focus {
z-index: z-index( '.editor-block-mover__control' );

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

Knowing z-index to be a source of maintenance headaches, I might like to see some inline comment either here or in z-index.scss explaining why it's needed. As it stands, I have no idea why we need it, even at this moment in time, let alone my future self.

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

👍👍 Adding a comment.

}
// Only show the left border if the Switcher is present
.editor-block-switcher + div > .components-toolbar {

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

This seems very prone to future breakage, particularly on the + div sibling part, since the div it refers to exists only for adding a left border in a set of toolbar controls. This has no reason for needing to be a div specifically, and could otherwise be refactored to a solution which no longer requires the div wrapper at all.

Instead, I'd like to see one of:

  • Adding a border-right to the Toolbar of the EditorBlockSwitcher
  • See what we can do about removing this "seemingly redundant wrapper" div and change selector to apply border-left on .editor-block-toolbar .components-toolbar:not( :first-child )
  • Add hasLeftDivider as a prop to control / override the existing behavior of Toolbar in applying the .has-left-divider modifier class

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

The first idea was a solid bit of advice. Did that:

border fix

Address feedback.
Add border to switcher component instead.

Add comment to explain z index.

@youknowriad youknowriad merged commit 5a57e6f into master Jun 1, 2018

2 checks passed

codecov/project 46.31% (-0.15%) compared to 0f620fc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/alternate-hover-approach branch Jun 1, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 1, 2018

🤘

@iseulde

This comment has been minimized.

Member

iseulde commented Jun 1, 2018

drag-label

Dragging seems broken here

@aduth

This comment has been minimized.

Member

aduth commented Jun 1, 2018

@iseulde If you're referring to the block label, dragging has never worked there. I know it's been mentioned a few times as being desirable, though unsure if there's an issue tracking it.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 1, 2018

Well, it does work for me — it's just that you have to click and drag and it's not a great experience yet.

It was my intention to create a separate ticket for this, including a ticket to improve the sibling inserter and theme the hover color, per feedback in this thread. But it's been a whirlwind so creating that ticket is still on my "todo". But the gist is I'm going to suppose sort of a rejiggering of dragondrop so it's only the label that becomes the drag handle, and not the sides. But I'm sure lots of discussion and idea refinement will ensue.

@iseulde

This comment has been minimized.

Member

iseulde commented Jun 1, 2018

The reason I'm mentioning it here is because the grabbing hand indicates that the block can be dragged by that label.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 1, 2018

Clicking and dragging that area is working for me, though, even if in a way that doesn't give the best visual feedback. Is it not for you?

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented Jun 1, 2018

@jasmussen Drag and drop from the hover title is not working for me either. (Tested in Firefox Nightly.) The cursor becomes the drag cursor when I hover over the title, but when I try to click and hold on the hover title, it just selects the block.

jasmussen added a commit that referenced this pull request Jun 1, 2018

Remove drag handle from breadcrumb
Per feedback in #6773 (comment), it seems the drag handle for the breadcrumb was premature. Although it works on the edge, it's not consistent and was a bad idea.

This simply removes the grab handle. I will open a separate ticket for making that work.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 1, 2018

It worked on the edge, but I see why there's confusion: it doesn't work on the whole breadcrumb. So yes, it was definitely too early to add the grab handle. I've opened #7083 PR to fix it.

jasmussen added a commit that referenced this pull request Jun 1, 2018

Remove drag handle from breadcrumb (#7083)
Per feedback in #6773 (comment), it seems the drag handle for the breadcrumb was premature. Although it works on the edge, it's not consistent and was a bad idea.

This simply removes the grab handle. I will open a separate ticket for making that work.
@for $i from 2 through 6 {
&.has-#{ $i }-columns {
&.has-#{ $i }-columns .editor-inner-blocks {

This comment has been minimized.

@aduth

aduth Jun 4, 2018

Member

This change seems to have inadvertently caused #7127, where the stylesheet is used for both the editor and front-end, but the markup generated from the front-end will not have .editor-inner-blocks.

This comment has been minimized.

@jasmussen

jasmussen Jun 4, 2018

Contributor

Looking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment