Add/drag handle #9569

oandregal · 2018-09-03T11:40:08Z

This PR adds the drag handle to the up/down mover.

TODO

Test

Regular drag-n-drop operation (including child blocks, and embeds).

mtias · 2018-09-03T11:45:33Z

packages/editor/src/components/block-mover/index.js

@@ -66,6 +66,30 @@ export class BlockMover extends Component {
 					onFocus={ this.onFocus }
 					onBlur={ this.onBlur }
 				/>
+				<Draggable


Thinking it might be worth setting this as an optional prop (default draggable to true) on the block mover.

This still needs to take into account the current BlockDraggable conditions. Are you thinking of something additional to those?

Yes, I was thinking something that would let you use the mover without drag handles for a certain UI if needed.

mtias · 2018-09-03T11:47:48Z

packages/editor/src/components/block-mover/index.js

+				>
+					{
+						( { onDraggableStart, onDraggableEnd } ) => (
+							<IconButton


Not sure we should use the button styles here. It looks a bit weird on hover.

I think the space should also be a bit tighter overall.

I'll leave @jasmussen apply his svg / css :magician: skills while I watch from the distance and learn.

oandregal · 2018-09-03T18:16:04Z

Also for design feedback: note that this PR also changes how Gutenberg indicates which block is being dragged: it uses the block's opacity instead of showing a grey box above the block.

In the spirit of doing one thing at a time, I wouldn't have changed that behavior in this PR if it wasn't for this: the grey box was a dedicated div that acted as a drag handle and that was made visible when the dragging process started; now that we've removed the dedicated drag handle we no longer have that div node to play with. Also: the opacity seems nicer to me in this case. If we still consider the grey box a preferred UI, I can figure out how to maintain that behavior.

jasmussen · 2018-09-04T08:31:04Z

Very nice work here.

I pushed a little polish in 4128f21:

That is, buttons are now slightly smaller so as to better fit on a single paragraph height, which is the smallest block we can have. The paragraph is actually 56px tall, and the buttons are now 72px (3*24px). So I've compensated by vertically centering them. Here's with a little debug help:

This is before:

I'm going to next take a stab at tweaking the hover style of the drag handle. It shouldn't be a button, the hand cursor itself should be mostly sufficient, perhaps with a color change on hover.

Also for design feedback: note that this PR also changes how Gutenberg indicates which block is being dragged: it uses the block's opacity instead of showing a grey box above the block

I think it's okay to visit this, it's a bit challenged as is.

The thing is, you can now style the editor... you can change the text color and the background color using editor styles. But unless @youknowriad with his new CSS parser can do magic here, we can't know which background color to show on a block that's being dragged. Until now we've simply assumed white.

Ideally we'd know what the color would be, it's a little hard to see that you're dragging a block right now since it has no background. But I think it's good to surface this discussion on what to do here.

Another option is, instead of showing a clone of the block, showing a small icon of the block. Like, imagine as you "lift the block off the page", it transforms into a small block next to your cursor, and expands again when placed. That way we'd cover less of the page. But I know @mtias has some opinions here we should listen to.

jasmussen · 2018-09-04T09:01:21Z

In 8c575d2 I committed a change I'd like a sanity check on. From the commit message:

Try: Remove hover style and make un-tabbable
This needs a sanity check and is a separate commit for that reason, so easier to revert if it isn't kosher.

This commit removes the hover and focus styles for the drag handle.

It also zeroes out the aria label, makes aria hidden, and unfocusable using the keyboard. The idea being that you cannot at all interact with the drag handle using just the keyboard, we have the movers for that, and thus this item shouldn't be announced to either screen readers or be part of the tab index as that's simply useless.

Thoughts?

Here's how this looks when mousing:

Here's how this looks when keyboarding:

Note by the way that in Chrome on Mac I'm seeing a "copy" cursor as soon as I start dragging. I should be seeing the grabbing cursor instead. Any insights as to why that might be?

jasmussen · 2018-09-04T09:07:01Z

Note by the way that in Chrome on Mac I'm seeing a "copy" cursor as soon as I start dragging. I should be seeing the grabbing cursor instead. Any insights as to why that might be?

Some further info on this, the copy cursor is supposed to show up in the file system while you're dragging a file and holding ⌥ at the same time.

Ironically, I'm experiencing the inverse behavior both in master and in this branch: when I'm holding ⌥, the copy cursor disappears and the correct grabbing cursor is present.

The behavior is correct in Safari — when I drag normally, I'm seeing the correct grabbing cursor. When I hold ⌥ in Safari, I'm seeing the copy cursor.

So it seems as though Chrome is either buggy, or my system is buggy, or some part of our code makes Chrome think that by default a drag action is a copy/duplicate action as opposed to a move action.

oandregal · 2018-09-04T09:31:32Z

In a96bb4b we experimented with setting the dnd event to move to see if that affected how chrome on mac styled things. It seems it does. It shows the default cursor instead (note how it briefly shows the move cursor and then the default one when the dragover event starts):

Change reverted in 3e595c5.

jasmussen · 2018-09-04T09:35:42Z

Removed the hover color from the drag handle, and also made it bg color friendly.

oandregal · 2018-09-04T16:02:35Z

Created an issue to track the cursor issue at #9610

afercia · 2018-09-04T16:53:12Z

The focusable attribute rendered in the HTML is invalid HTML and should be omitted:

Error: Attribute focusable not allowed on element button at this point.

jasmussen · 2018-09-05T07:03:21Z

The focusable attribute rendered in the HTML is invalid HTML and should be omitted:

Good catch, thanks. Is this implicit approval of the assertion that the drag handle should not be part of the tab order/be keyboardable?

afercia · 2018-09-05T07:19:32Z

Yup, I'd agree taking it out from the tab sequence and hiding it with aria-hidden=true makes sense, as it can't be used with a keyboard.

oandregal · 2018-09-05T16:11:51Z

@jasmussen in 8ee6afa I've changed the IconButton (which ended up adding a button element) by a simple div for the drag handle. The reason is: Firefox won't make buttons draggable. Would you mind giving it a sanity check and verify if it unintentionally broke any of your modifications?

afercia · 2018-09-05T18:12:43Z

in 8ee6afa I've changed the IconButton (which ended up adding a button element) by a simple div

Then, I'd suggest to remove tabIndex, as a div is not focusable by default. Also onFocus / onBlur I guess. And the empty aria-label="" is a bit pointless.

jasmussen · 2018-09-06T06:47:18Z

I removed the aria label and tabindex.

I would honestly prefer we remove the tooltip as well — the drag handle feels obvious to me, and because it is an addition for mouse users to the clickable up/down arrows, it doesn't feel necessary to me.

On the flipside, it feels intrusive, and often invokes even when it shouldn't:

— at this point I'm dragging, so the tooltip should disappear. In other words, if we are to have a tooltip, we need to make sure it disappears as soon as you start dragging.

I also feel like we should consider doing something about the transparent background while dragging. I'm going to try something, but not sure it'll work.

afercia · 2018-09-06T07:15:13Z

Not sure why onFocus and onBlur are kept, as this <div> can't be receive focus and can't be "blurred".

jasmussen · 2018-09-06T08:38:30Z

@nosolosw I'll let you respond to the onfocus thoughts.

For now, I've made a change to the visual styling of the dragondrop:

I know this is kind of like how it used to be. I'd like to explain why it is this way. Most importantly: we are not cloning a block when you are dragging, we are moving, so it's important the block you move is not seen as a clone. Hence the gray BG.

Also, I hid the block toolbar and mover on the "block that's left behind", it didn't make sense to keep those there. But I am hiding them on the dragged block.

I tried unhiding the editor-block-mover on the draggable clone — because otherwise the grip you're holding disappears when you are dragondropping, that's a little weird. But doing so makes dragondrop not work. Any idea why?

So drag is something that can be turned on/off depending on different UI needs.

The drag handle is part of the mover so we want to keep it visible while dragging.

This makes BlockDraggable DOM agnostic and uses it as a base to compose drag data. The vision is that we may have an ImageDraggable component in the future that would abstract what data the dragging operation needs to drag images, and so on. The goal would be to use it by only providing one prop, the blockClientId. Making this a single commit in case we want to revert.

designsimply · 2018-09-14T18:17:40Z

I noticed neither the draggable handle nor the movers are available in spotlight mode, and I wanted to check whether that is the intended behavior?

Every so tiny design nitpick: I noticed the icon for the drag handle (6 dots) is not the same shade of gray compared to the mover icons and it seemed to me they should be the same color. However, I also thought the color difference might intentional to make the draggable handle stand out a bit. (screenshot)

Fixes design nitpick from #9569 (comment).

jasmussen · 2018-09-17T08:25:56Z

I noticed neither the draggable handle nor the movers are available in spotlight mode, and I wanted to check whether that is the intended behavior?

Yes, that's intentional. It's more of a mode for writing and focusing, than it is for rearranging. But thanks for checking int.

Every so tiny design nitpick: I noticed the icon for the drag handle (6 dots) is not the same shade of gray compared to the mover icons and it seemed to me they should be the same color. However, I also thought the color difference might intentional to make the draggable handle stand out a bit.

EAGLE EYE SHERI! Awesome.

PR here: #9948

youknowriad · 2018-09-17T08:49:03Z

packages/editor/src/components/block-list/style.scss

-
-		.is-dark-theme & {
-			background-color: $light-opacity-light-200;
+.editor-block-list__layout .editor-block-list__block.is-selected { // Needs specificity to override inherited styles.


It looks like the changes here introduced a regression in Spotlight mode.

It should be fixed here #9951

Fixes design nitpick from #9569 (comment).

StaggerLeee · 2018-11-01T09:25:43Z

Just one note. Nothing for new issue really.

When you are testing your development test allways with all Admin colors (User profile settings).
Few of the colors are "weak", and drag & drop line is not strong visible as in default blue color.

oandregal changed the base branch from master to add/with-draggable September 3, 2018 11:40

oandregal requested a review from jasmussen September 3, 2018 11:41

oandregal self-assigned this Sep 3, 2018

oandregal added the [Feature] Drag and Drop Drag and drop functionality when working with blocks label Sep 3, 2018

mtias reviewed Sep 3, 2018

View reviewed changes

oandregal force-pushed the add/with-draggable branch from 53b10d3 to 755e0aa Compare September 3, 2018 11:45

mtias reviewed Sep 3, 2018

View reviewed changes

mtias added this to the 3.8 milestone Sep 3, 2018

oandregal force-pushed the add/drag-handle branch from 3fd4c44 to 1895044 Compare September 3, 2018 15:49

oandregal changed the base branch from add/with-draggable to master September 3, 2018 15:51

oandregal mentioned this pull request Sep 4, 2018

Incorrect dragging icon in chrome (some platforms) #9610

Closed

youknowriad modified the milestones: 3.8, 3.9 Sep 5, 2018

oandregal added the [Status] In Progress Tracking issues with work in progress label Sep 5, 2018

oandregal force-pushed the add/drag-handle branch from de12ca4 to 48b8fd5 Compare September 6, 2018 09:49

oandregal and others added 14 commits September 14, 2018 10:15

Add tooltip

59639b1

Expose an isDraggable property to control draggability

070b059

So drag is something that can be turned on/off depending on different UI needs.

Remove empty aria label and tabindex.

043e191

Try a different style for moving.

ac3f475

Make the gray spot only fit the content.

dca05f5

Update tests

172daa5

Remove tooltip

4cbbfc5

Remove unnecessary blur/focus handlers

638393a

Remove no longer necessary code

6901e69

Show the mover in the clone image

48d0d6f

The drag handle is part of the mover so we want to keep it visible while dragging.

Get index and rootClientId from state

42ed441

Rename arrows to icons

e9f994c

Use AccesibleSVG component

f253641

oandregal force-pushed the add/drag-handle branch from b8061f8 to f253641 Compare September 14, 2018 08:59

oandregal merged commit 62c527f into master Sep 14, 2018

oandregal deleted the add/drag-handle branch September 14, 2018 09:11

jasmussen added a commit that referenced this pull request Sep 17, 2018

Fix draghandle color

c596993

Fixes design nitpick from #9569 (comment).

jasmussen mentioned this pull request Sep 17, 2018

Fix draghandle color #9948

Merged

youknowriad mentioned this pull request Sep 17, 2018

Spotlight mode: Fix inactive block's styling regression #9951

Merged

youknowriad reviewed Sep 17, 2018

View reviewed changes

jasmussen added a commit that referenced this pull request Sep 17, 2018

Fix draghandle color (#9948)

41cb78b

Fixes design nitpick from #9569 (comment).

oandregal mentioned this pull request Sep 18, 2018

Drag & Drop: Change to use hover label as the draggable handle #7114

Closed

mcsf pushed a commit that referenced this pull request Sep 21, 2018

Fix draghandle color (#9948)

0b7e8b8

Fixes design nitpick from #9569 (comment).

oandregal mentioned this pull request Sep 26, 2018

Visibility conditions for the drag handle in child blocks #10188

Closed

myleshyson mentioned this pull request Mar 27, 2019

Add option to keep drag handles in spotlight mode #14670

Closed

oandregal mentioned this pull request Nov 28, 2019

Refactor the draggable component to avoid passing around unnecessary props #18756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/drag handle #9569

Add/drag handle #9569

oandregal commented Sep 3, 2018 •

edited

Loading

mtias Sep 3, 2018

oandregal Sep 3, 2018 •

edited

Loading

mtias Sep 3, 2018

mtias Sep 3, 2018

mtias Sep 3, 2018

oandregal Sep 3, 2018

oandregal commented Sep 3, 2018

jasmussen commented Sep 4, 2018

jasmussen commented Sep 4, 2018

jasmussen commented Sep 4, 2018

oandregal commented Sep 4, 2018 •

edited

Loading

jasmussen commented Sep 4, 2018

oandregal commented Sep 4, 2018

afercia commented Sep 4, 2018

jasmussen commented Sep 5, 2018

afercia commented Sep 5, 2018

oandregal commented Sep 5, 2018

afercia commented Sep 5, 2018

jasmussen commented Sep 6, 2018

afercia commented Sep 6, 2018

jasmussen commented Sep 6, 2018

designsimply commented Sep 14, 2018

jasmussen commented Sep 17, 2018

youknowriad Sep 17, 2018

StaggerLeee commented Nov 1, 2018

Add/drag handle #9569

Add/drag handle #9569

Conversation

oandregal commented Sep 3, 2018 • edited Loading

TODO

Test

mtias Sep 3, 2018

Choose a reason for hiding this comment

oandregal Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

mtias Sep 3, 2018

Choose a reason for hiding this comment

mtias Sep 3, 2018

Choose a reason for hiding this comment

mtias Sep 3, 2018

Choose a reason for hiding this comment

oandregal Sep 3, 2018

Choose a reason for hiding this comment

oandregal commented Sep 3, 2018

jasmussen commented Sep 4, 2018

jasmussen commented Sep 4, 2018

jasmussen commented Sep 4, 2018

oandregal commented Sep 4, 2018 • edited Loading

jasmussen commented Sep 4, 2018

oandregal commented Sep 4, 2018

afercia commented Sep 4, 2018

jasmussen commented Sep 5, 2018

afercia commented Sep 5, 2018

oandregal commented Sep 5, 2018

afercia commented Sep 5, 2018

jasmussen commented Sep 6, 2018

afercia commented Sep 6, 2018

jasmussen commented Sep 6, 2018

designsimply commented Sep 14, 2018

jasmussen commented Sep 17, 2018

youknowriad Sep 17, 2018

Choose a reason for hiding this comment

StaggerLeee commented Nov 1, 2018

oandregal commented Sep 3, 2018 •

edited

Loading

oandregal Sep 3, 2018 •

edited

Loading

oandregal commented Sep 4, 2018 •

edited

Loading