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

Drag and Drop #1733

Merged
merged 45 commits into from
Apr 3, 2019
Merged

Drag and Drop #1733

merged 45 commits into from
Apr 3, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Mar 14, 2019

Summary

Reference: #1651

Drag and Drop support consisting of 3 main components (The docs are the best place to look for descriptions and intended use for each):

  • EuiDragDropContext
  • EuiDraggable
  • EuiDroppable

Help Wanted

💡 Ideas for improvement

  • More props for styling purposes?
  • More EUI-specific functionality opinions?

👩‍🎨 Design

  • Docs about expected draggable item design?
  • Dark mode scheme?

🔮 Future

  • EUI convenience components to make common cases easier (e.g., reorderable list)
  • Note the early nature of these components and their APIs (i.e., 'beta'-like tag in the docs)?

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios

- [ ] This required updates to Framer X components

@thompsongl
Copy link
Contributor Author

Naturally, the latest minor release of react-beautiful-dnd breaks my clone/copy extension.
I'm going to rework it before taking this PR out of Draft status.

@thompsongl thompsongl marked this pull request as ready for review March 20, 2019 16:35
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Starting with review on docs before deep diving into all the codes

@cchaos
Copy link
Contributor

cchaos commented Apr 2, 2019

I think it's mostly good as a beta. But I'd still like to work on the removal option.

  1. Why does the drop panel shift in size when dragging over?
  2. What can we add to signify removal when dragging outside of drop area?
  3. Why is there a flash of the item inside of the drop area again before being removed?

@thompsongl
Copy link
Contributor Author

@cchaos 1 & 3 will require some more CSS animation hacking. I'll look into those.
Any ideas for 2?

@cchaos
Copy link
Contributor

cchaos commented Apr 2, 2019

I was hoping for a simple cursor type change, but there's nothing that suits. What about doing something like adding a badge that says "remove" once you're outside of the bounds?

@snide @ryankeairns Thoughts?

@ryankeairns
Copy link
Contributor

@cchaos is that a destructive action or does the item get placed back into the left hand list?

@cchaos
Copy link
Contributor

cchaos commented Apr 2, 2019

@ryankeairns Depends on the consumer, but most likely undoable. We just need to signify that dragging outside of the droppable area would result in removal of the item.

@thompsongl
Copy link
Contributor Author

Why does the drop panel shift in size when dragging over?

Flexbox plus a width discrepancy because of the panel border. Fixed

Why is there a flash of the item inside of the drop area again before being removed?

react-beautiful-dnd native drop animation. What I did was add a prop to EuiDraggable that removes the drop animation and used onDragUpdate to determine whether to set that prop.

@ryankeairns
Copy link
Contributor

Thoughts about indicating removal from simple to more complex:

  • slightly scale down the size when its outside
  • turn the cross icon into a trash can when its outside (or maybe hide the cross on drag until you go outside the droppable area, then show it again)
  • when you drop outside (let go), show a little trash animation

trash

@cchaos
Copy link
Contributor

cchaos commented Apr 2, 2019

Thanks @ryankeairns

slightly scale down the size when its outside
I had no idea you could do that in Sketch, I just always unchecked the option then hit the trashcan.

That being said, I'm not really sure how scaling indicates removal. Seems like an arbitrary animation

turn the cross icon into a trash can when its outside (or maybe hide the cross on drag until you go outside the droppable area, then show it again)

The cross is just an example of how to add distinguished buttons for removing options. It's not forced by the component.

when you drop outside (let go), show a little trash animation

I thought about this as well, but it's a bit beyond this scope to create that atm.

Any thoughts on the badge idea?

@ryankeairns
Copy link
Contributor

@cchaos The badge works well enough. It's an atypical use of a badge, but then it literally says 'Remove' on it and it's red, both clearly foretelling what's about to happen.

As for the shrinking, it implies that the held item has left the dropzone and it could be further animated upon release where it shrinks and fades to nothingness. In that way, the initial scaling indicates pending removal.

@cchaos
Copy link
Contributor

cchaos commented Apr 2, 2019

Hmmm, adding that extra animation of shrinking until non-existent could make that initial shrink more intentional. @thompsongl By looking at your overriding of the transforms do you think this would even be possible?

@ryankeairns
Copy link
Contributor

One last thought, maybe just show a trash can icon instead of a badge. What you have works, I thought you were looking for other ideas :)

@cchaos
Copy link
Contributor

cchaos commented Apr 2, 2019

FYI: We've decided to punt on figuring out the design of removing an item as it'll be an additive feature and don't want to hold up this PR for that.

@chandlerprall
Copy link
Contributor

chandlerprall commented Apr 3, 2019

I wanted to prove out that table dragging can be achieved with little changes (and no breaking) to this setup. chandlerprall@3b1ac3a

table row dragging

Changes:

  • added As prop to EuiDraggable to allow rendering tr instead of div, added support for multiple children (multiple td children in the tr)
  • augmented EuiBasicTable and EuiTableRow to take a dragAndDrop prop which informs inserting the EuiDraggable and EuiDroppable components as needed

My changes aren't production-ready, but I feel it proves a future approach well enough to not block this PR.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM (as beta) 🎉

@thompsongl thompsongl merged commit 1b200b1 into elastic:master Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants