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

Use the breadcrumb as the draggable handle #8764

Closed
wants to merge 21 commits into from

Conversation

@nosolosw
Copy link
Member

nosolosw commented Aug 9, 2018

This PR aims to address #7114

It introduces a new withDraggable component that decouples the drag handle (in this example, the breadcrumb/hover label) from the element being dragged (in this example, the block). Unlike the existing Draggable component, withDraggable is not concerned with creating a DOM node that serves as drag handle; it's the wrapped component responsibility to do so, which makes withDraggable reusable in several use cases.

An example of how to use it:

  • wrap the drag handle.
  • initialize it with the initDragging prop exposed by the withDraggable HOC with the DOM id of the element to drag and the necessary transfer data.
import { Dashicon, Panel, PanelBody, withDraggable } from '@wordpress/components';

const MyDraggable = ( { onDragStart } ) => (
  <div id="draggable-panel">
    <Panel header="Draggable panel" >
      <PanelBody>
        <Dashicon
          icon="move"
          draggable="true"
          onDragStart={ initDragging( 
              "draggable-panel",
              { /* data to be transfered goes here */ }
          ) }
      />
      </PanelBody>
    </Panel>
  </div>
);

export default withDraggable( MyDraggable );

Current status / Open questions

  1. For the breadcrumb to be able to handle the drag events, it needs to be visible when is clicked (now it's only visible on hover). The current status is that is always visible when the block is selected. A potential alternative is to make it visible on hover + when it is the target of a click event.

peek 2018-08-21 13-53

  1. The classic block is a bit different. It doesn't show the breadcrumb. We either need an alternative handle for that (the toolbar itself?) or make it visible. This PR makes the breadcrumb visible on the classic block, just for demonstrating how bad that would be. Due to how that block behaves (it shows always a toolbar than it's converted to a mce editor on the click event) we would need to some refactorings before we are able to use the withDraggable component in that particular case:

peek 2018-08-21 13-55-classic

TODO

  • Improve the breadcrumb draggable indicator (now it's using a hack on the compiled dashicon component).
  • Add back a placeholder in the block that is being dragged. Because the handle is no longer overlapped with the block content, we need a replacement for this (we used to make the draggable area visible as a placeholder when detecting that it was being dragged).
  • Make the breadcrumb and the draggable component areas equal. At the moment they're separated entities, use the draggable to wrap the breadcrumb. If we keep them separated, we could render the breadcrumb text also in the draggable component, to make the area equal.
  • Separate the drag handler from the logic to add a drag image and setting the drag data.
  • Adjust z-index to prevent the breadcrumb from showing on top of popovers.

Test

Drag and drop a block and test that it works as used to be. Specifically test: embeds (or anything with iframes), nested blocks.

  • Chrome
  • Firefox
  • IE 11
  • IE Edge
  • Safari

@nosolosw nosolosw self-assigned this Aug 9, 2018

@nosolosw nosolosw changed the title Update/dnd handle Change the draggable handle Aug 9, 2018

@nosolosw nosolosw changed the title Change the draggable handle Use the breadcrumb as the draggable handle Aug 9, 2018

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 9, 2018

Current status:

peek 2018-08-09 13-24

@nosolosw nosolosw force-pushed the update/dnd-handle branch from 9a06f5d to c5e55f8 Aug 9, 2018

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 9, 2018

Status after c5e55f8:

peek 2018-08-09 18-11

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Aug 9, 2018

I would like it if the block controls doubled as drag handles. That would provide a way to drag the selected block.

Noting that the outcome of #6224 may make this redundant and remove the need for it, but until then, I think this is a good improvement over master. I really dislike the current invisible drag handles.

@nosolosw nosolosw force-pushed the update/dnd-handle branch from c5e55f8 to a9dc4ae Aug 10, 2018

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 10, 2018

a9dc4ae implements a proof of concept for the draggable indicator using dashicons

peek 2018-08-10 11-23

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 13, 2018

In fbd9d0d we experiment with the block's opacity to communicate what is being dragged instead of showing a grey box on top of the block. This approach is nicer, and also solves some technical challenges of the refactoring (see commit's comment).

peek 2018-08-13 17-53

@@ -15,12 +15,13 @@ export default class Dashicon extends Component {
return (
this.props.icon !== nextProps.icon ||
this.props.size !== nextProps.size ||
this.props.viewBox !== nextProps.viewBox ||

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 17, 2018

Author Member

These changes to the dashicon component is an interim hack. We need to either port them to upstream dashicon repo if they make sense or figure out a different approach to show the drag affordance to the user.

@@ -985,6 +908,10 @@
.editor-block-list__block:hover & {
@include fade_in(0.1s);
}

.editor-block-list__breadcrumb-drag-handle {

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 17, 2018

Author Member

This is part of the dashicon hack to show a drag affordance to user. See https://github.com/WordPress/gutenberg/pull/8764/files#r210868679

@@ -95,7 +24,7 @@
.editor-block-list__block {
&.is-hidden *,
&.is-hidden > * {
visibility: hidden;

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 17, 2018

Author Member

Previous to this PR, the block's contents were hidden and a grey box was made visible on top of them instead. That grey box was a div element created by the Draggable and BlockDraggable components. If we want to keep that pattern we'll need to figure out something for this as we no longer have a separate DOM component as the drag handle.

Lowering the opacity of the content being dragged is also a nice pattern other apps use and helps in identifying what's being dragged.

nosolosw added some commits Aug 9, 2018

We're going to get rid of side grabs
So we don't need these.
Dont make the handle ever visible.
The drag handle no longer overlaps the block content div,
so we can't render a placeholder in the handle itself.
We need to figure somethingelse out.
HACK: PoC to improve the draggable indicator
This tweaks the dashicon viewBox to render a not too embarrasing
draggable indicator. This needs fixing.
Use opacity to communicate what block is being dragged.
Previous to this refactor, it used to be a div inside the block wrapper
that would become visible when the block was being dragged. That
div would appear as grey. By refactoring the drag handler to the
breadcrumb we no longer have that div, so we seek to use
a different technique to communicate the same behavior.

I also think is nicer to show the original content.

nosolosw added some commits Aug 17, 2018

Remove block-draggable
it's only used internally by the block-list component.
withDraggable injects initDragging instead of onDragStart
The new name clarifies the purpose and allows the original component
to have passed the onDragStart property by some parent component.

@nosolosw nosolosw force-pushed the update/dnd-handle branch from 157cb2c to 7c37b06 Aug 20, 2018

@@ -9,7 +9,7 @@ $z-layers: (
".editor-block-list__block {core/image aligned left or right}": 20,
".editor-block-list__block {core/image aligned wide or fullwide}": 20,
".freeform-toolbar": 10,
".editor-block-list__breadcrumb": 1,
".editor-block-list__breadcrumb": 1000000000, // used as a drag handler, we want it to be shown above the entire UI

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 20, 2018

Author Member

Adjust this number to avoid conflicts with popovers:

popover-small-screen-conflict

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 20, 2018

Author Member

And with the classic block:

classic-breadcrumb

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 21, 2018

Author Member

9999a4f fixes the problem with the popover.

@@ -394,7 +393,7 @@ export class BlockListBlock extends Component {
// We render block movers and block settings to keep them tabbale even if hidden
const shouldRenderMovers = ( isSelected || hoverArea === 'left' ) && ! showEmptyBlockSideInserter && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock;
const shouldRenderBlockSettings = ( isSelected || hoverArea === 'right' ) && ! isMultiSelecting && ! isPartOfMultiSelection;
const shouldShowBreadcrumb = isHovered && ! isEmptyDefaultBlock;
const shouldShowBreadcrumb = ( isSelected || isHovered ) && ! isEmptyDefaultBlock;

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 20, 2018

Author Member

This makes the hover label to stick when the block receives a click, and making it visible when a block is being edited:

hover-on-block-edit

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Aug 20, 2018

Contributor

I think this is a bad idea. Nothing should ever constantly overlap the content of the selected block. It is bad enough that sibling inserters currently do this; see #8206, #8881, and #8883.

If the hover label was shown outside of the block border, then I think this would be fine. Of course, that would conflict with the toolbar. I think that dragging a selected block should be done from empty space in the toolbar (or just dragging from anywhere in the toolbar, similar to what GNOME does), and/or dragging from the up/down movers on the side.

See also:

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 21, 2018

Author Member

This PR is at a point where it's functional and needs design review, that's indeed one of the things we need to discuss and iterate on. I've pinged @jasmussen for that.

This comment has been minimized.

Copy link
@chrisvanpatten

chrisvanpatten Aug 21, 2018

Member

Just want to note I'm also pretty uncomfortable with the handle being visible when the block is selected. Feels like additional weight in the UI.

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 21, 2018

@jasmussen would love your feedback on the open questions, design/interaction wise. #9074 may be potentially related (I commented there as well).

@youknowriad @aduth I saw you were the Gutenberg core reviewers in #4115, the PR that first implemented drag and drop. I would appreciate any thoughts you may have on this PR's direction and the withDraggable HOC.

@nosolosw nosolosw requested review from jasmussen, aduth and youknowriad Aug 21, 2018

const cloneHeightTransformationBreakpoint = 700;
const clonePadding = 20;

const withDraggable = createHigherOrderComponent(

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 21, 2018

Author Member

FWIW, this component ports the existing Draggable functionality (set data transfer and create a drag image).

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Aug 21, 2018

Noting that #9197 is related as it affects the hover label.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Aug 22, 2018

Nice work! Thank you for working on this.

Here's a GIF, that's always helpful:

dragondrop

The good:

  • This is working well
  • It's completely fine to just have one spot for dragondrop

What we need to work out:

  • We can't have the hover label keep showing as the block is selected.
  • Because we can't have that, the dragondrop feature might appear to only work when you have no blocks selected.
  • There seems to be TWO spots to drop the block in between all blocks, not just one. Not sure why this is.

I'm personally of the opinion that drag and drop is very nice for a few things, like reordering items in a ToDo list, but for larger operations it's super fiddly and prone to error. For that specific reason I'm personally totally fine with drag and drop being purely a power-user feature that isn't made particularly visible by drag handles or affordances when a block is selected.

But the hover/select issue is nonetheless something to think about. Flagging @karmatosed and @mtias for thougths here. Should we make the up/down movers double as dragondrop handles? Are there other solutions?

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 22, 2018

There seems to be TWO spots to drop the block in between all blocks, not just one. Not sure why this is.

This is the existing behavior on Gutenberg master as well. The reason is: the drop behavior is part of the block logic. And because you can drop a block above or below any other existing one, when you are dropping between blocks you'll have two spots: the below drop spot of one block, and the above drop spot of the next one.


Edit: there is a potential fix that keeps the logic at the block level. We'd need to modify the drop zone behavior to only offer the spot below the block by default. It'd show the spot on top of the block if it is the first one (I assume we can know this by the block order prop).

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Aug 22, 2018

Ah, thanks for clarifying. This seems worth it for us to fix separately. But yes definitely out of scope for this PR.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Aug 22, 2018

@jasmussen

Should we make the up/down movers double as dragondrop handles? Are there other solutions?

I am definitely in favor of making the up/down movers double as drag-and-drop handles. The block toolbar could also double as a drag handle. GNOME does this with its header bars and I think it works quite well.

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 27, 2018

Closing this in favor of a more step-by-step process. The first step is to refactor how drag-n-drop works internally without changing the external behavior, see #9311

When that PR lands we can discuss in a separate PR what element(s) should become the new drag handle(s). That also buy us some time to fix some existing drop behaviors in parallel, and explore/consolidate changes to UI that are being discussed in other PRs (and affect to dnd behavior).

@nosolosw nosolosw closed this Aug 27, 2018

@nosolosw nosolosw deleted the update/dnd-handle branch Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.