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.

Sign up for GitHub

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

Jump to bottom

[CARBON-865] CustomDragLayer component for enabling Drag & Drop ghost row #1439

Merged
merged 10 commits into from Aug 29, 2017
Merged

[CARBON-865] CustomDragLayer component for enabling Drag & Drop ghost row #1439

merged 10 commits into from Aug 29, 2017

Conversation

paulsturgess
Copy link

@paulsturgess paulsturgess commented Jul 26, 2017 •
edited

Ticket CARBON-865

Implemented by using: https://react-dnd.github.io/react-dnd/docs-drag-layer.html

Chrome
drag-chrome

Firefox
drag-ff

Edge
drag-edge

TODO:

  • Figure out how we can handle the width of the ghost row
  • Specs
  • Add the ghost row to the drag and drop demo
  • Changelog

@paulsturgess paulsturgess self-assigned this Jul 26, 2017
@SeanArmstrong
Copy link

SeanArmstrong commented Jul 27, 2017 •
edited

Went to have a play with it and got this error - react-dnd/react-dnd#431

Caused by this - https://github.com/react-dnd/react-dnd/blob/ca10e688651833d10b378a2a1bf284a0264ed4c8/packages/react-dnd/src/DragLayer.js#L96-L105

Possible workaround at the bottom of the issue

@paulsturgess
Copy link
Author

@SeanArmstrong thanks for the heads up, I've managed to remove the error. I've updated the CustomDragLayer to pass through a prop of customDragLayer: true. This allows the developer to change the behaviour of the component used for the ghost row (the child of CustomDragLayer).

So for the ConfigurableItemRow, I have updated it to check if the prop is true, and if so, remove any references to WithDrop and WithDrag.

It might still be worth splitting ConfigurableItemRow into two components, one draggable and one not draggable so that it is not littered with if (this.props.customDragLayer). I'd need to split the icon and the list item into stand-alone components, to make them easy to compose.

@andrewjtait andrewjtait added this to the v1.5.0 milestone Jul 27, 2017
@paulsturgess paulsturgess force-pushed the CARBON-865 branch 5 times, most recently from b8ebf58 to f2a9669 Compare July 28, 2017 12:01
@paulsturgess
Copy link
Author

Work on this has paused due to the difficulty of placing the ghost layer inside the table which renders the html markup invalid.

@paulsturgess paulsturgess force-pushed the CARBON-865 branch 7 times, most recently from 8278a82 to 6f54adf Compare August 9, 2017 11:30
@paulsturgess
Copy link
Author

This has now been refactored so that the <CustomDragLayer /> can be defined outside the table, which means the markup remains valid.

Tested on the following browsers:

  • Mac Chrome, Safari & Firefox
  • Windows 8.1 IE 11
  • Windows 10 MSEdge

@paulsturgess paulsturgess force-pushed the CARBON-865 branch 2 times, most recently from dc3d219 to d5c64ea Compare August 10, 2017 08:18
@paulsturgess paulsturgess changed the title CustomDragLayer component for enabling Drag & Drop ghost row [CARBON-865] CustomDragLayer component for enabling Drag & Drop ghost row Aug 10, 2017
@paulsturgess
Copy link
Author

Unfortunately Safari is not displaying the 'grabbing' cursor when dragging and instead shows the text cursor.

@paulsturgess paulsturgess force-pushed the CARBON-865 branch 2 times, most recently from f988c31 to 2e5ed88 Compare August 10, 2017 15:05
@paulsturgess
Copy link
Author

I have raised separate tickets for the issue with the Safari cursor and the performance of the DraggableContext component on the Carbon site. CARBON-942 and CARBON-939.

SeanArmstrong
SeanArmstrong reviewed Aug 14, 2017
View reviewed changes
src/components/drag-and-drop/draggable-context/draggable-context.js Outdated
return (
<div className='carbon-draggable-context'>
{ this.props.children }
<CustomDragLayer />
Copy link

Choose a reason for hiding this comment

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

Could this pass some DragLayerProps so that its easier to change the custom drag layer.

Or enable a way so that you can optionally pass your own custom drag layer.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I like the idea of being able to pass in your own drag layer into the Draggable Context.

CustomDragLayer component for enabling Drag & Drop ghost row
19b052b 
In order to enable this you need to define the `draggableNode` prop on
the `<WithDrag>` component. For example:

```
class DraggableItems extends React.Component {
  render() {
    return (
      <DraggableContext onDrag={ onItemMoved }>
        <ol>
          {
            items.map((item, index) => {
              return (
                <WithDrop key={ index } index={ index }>
                  <DraggableItem />
                </WithDrop>
              );
            });
          }
        </ol>
      </DraggableContext>
    );
  }
}

...

class DraggableItem extends React.Component {
  render() {
    return (
      <li ref={ (node) => { this._listItem = node; } } >
        <WithDrag draggableNode={ () => { return this._listItem; } }>
          <span>{ item.content }</span>
        </WithDrag>
      </li>
    );
  }
}
```

Note that the `draggableNode` is passed as a function because the ref
`_listItem` is undefined until the component is mounted.
@andrewjtait andrewjtait modified the milestones: v1.6.0, v1.5.0 Aug 24, 2017
CustomDragLayer component for enabling Drag & Drop ghost row
1b36110 
In order to enable this you need to define the `draggableNode` prop on
the `<WithDrag>` component. For example:

```
class DraggableItems extends React.Component {
  render() {
    return (
      <DraggableContext onDrag={ onItemMoved }>
        <ol>
          {
            items.map((item, index) => {
              return (
                <WithDrop key={ index } index={ index }>
                  <DraggableItem />
                </WithDrop>
              );
            });
          }
        </ol>
      </DraggableContext>
    );
  }
}

...

class DraggableItem extends React.Component {
  render() {
    return (
      <li ref={ (node) => { this._listItem = node; } } >
        <WithDrag draggableNode={ () => { return this._listItem; } }>
          <span>{ item.content }</span>
        </WithDrag>
      </li>
    );
  }
}
```

Note that the `draggableNode` is passed as a function because the ref
`_listItem` is undefined until the component is mounted.
@ianrobsonsage
Copy link
Contributor

Re-QA'd after further improvements have been made.

As previously fine in all browsers other than the Safari icon issue already logged. QA Passed.

andrewjtait
andrewjtait suggested changes Aug 24, 2017
View reviewed changes
Copy link
Contributor

@andrewjtait andrewjtait left a comment

Choose a reason for hiding this comment

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

will wait to see if we get anywhere with #1494 before merging

Paul Sturgess and others added 4 commits August 24, 2017 16:34
Prevent Safari from changing the mouse cursor on drag
aeed752 
Use 'selectstart' event listener to call preventDefault if the user is
dragging a dom node or is already dragging.
Merge branch 'CARBON-865' into CARBON-865-safari-fix
ab8187e
with drag safari bug coverage
b2dba8b
@andrewjtait
Merge pull request #1494 from Sage/CARBON-865-safari-fix
f33a17f 
Fix Safari cursor for drag and drop
@SeanArmstrong SeanArmstrong merged commit c1cd931 into master Aug 29, 2017
@SeanArmstrong SeanArmstrong deleted the CARBON-865 branch August 29, 2017 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewjtait andrewjtait andrewjtait approved these changes

@SeanArmstrong SeanArmstrong SeanArmstrong approved these changes

Assignees

@paulsturgess paulsturgess

Labels
Passed QA Passed Review
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@paulsturgess @SeanArmstrong @ianrobsonsage @andrewjtait