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

Improve reordering with hugely mixed size Draggables #930

Closed
rd-dev-ukraine opened this issue Nov 15, 2018 · 51 comments
Closed

Improve reordering with hugely mixed size Draggables #930

rd-dev-ukraine opened this issue Nov 15, 2018 · 51 comments

Comments

@rd-dev-ukraine
Copy link

Bug or feature request?

Bug.

In case of mixing large-size draggables and small-size draggables we got a crazy behaviour.

Expected behavior

I can properly rearrange large draggables and this should look nice (like on small draggables)

Actual behavior

In my real application - I can't rearrange large draggables. Medium-sized draggables can be rearranged only if user will cross another item with center of current draggable. The problem is very huge, because only part of draggable does have dragHandleProps.
In my sample - it just looks crazy and it's non-trivially to rearrange large draggable, but possible under specific user action.

Steps to reproduce

  1. Run demo link;
  2. Try to move "Item 2" (large block) between "Item 0" and "Item 1" in one action (it's impossible)
  3. Try to move "Item 2" before "Item 0" in one action (sometime it works, sometime it doesn't)

What version of React are you using?

16.4.1

What version of react-beautiful-dnd are you running?

10.0.0

What browser are you using?

Chrome (latest stable)

Demo

https://codesandbox.io/s/qv629nm036

Thoughts

It looks like the problem happens because react-beautiful-dnd uses center of draggable as control point.
This is especially incorrect in case of drag handles. And even without drag handles it's better to use mouse position inside of draggable as control point

@alexreardon
Copy link
Collaborator

We use the center position as the control point as it is the items center of mass.

But the example you are showing does have some lame interactions. So it is worth looking into a bit more

@alexreardon alexreardon changed the title Issues with dragging large draggables Improve reordering with hugely mixed size Draggables Nov 21, 2018
cbothner added a commit to galahq/gala that referenced this issue Nov 22, 2018
Blocked on atlassian/react-beautiful-dnd#930
for actual mouse-driven drag and drop (since the size of a card is too
large relative to the table of contents), but using the keyboard mode
this works. Drag a card onto a page in the TOC to move it to that page.
Or drag it between two entries in the TOC to create a new page at that
location.
cbothner added a commit to galahq/gala that referenced this issue Nov 22, 2018
Blocked on atlassian/react-beautiful-dnd#930
for actual mouse-driven drag and drop (since the size of a card is too
large relative to the table of contents), but using the keyboard mode
this works. Drag a card onto a page in the TOC to move it to that page.
Or drag it between two entries in the TOC to create a new page at that
location.
@gabrielenosso
Copy link

I have a similar problem:
I have the same object that can be rendered in 3 different sizes.
In the destination, they are all shown as small.
When I drag it from the source, independently from the size, I show it as small.

So the problem is that it looks weird if I drag the Big object and then I show it as Small during the drag operation. (the position relative to the mouse cursors is broken)

@gihrig
Copy link

gihrig commented Dec 11, 2018

Similar issue, more discussion at #978

@gihrig
Copy link

gihrig commented Dec 11, 2018

@alexreardon How difficult would it be to create a config item to place the active point of a draggable at its "center of gravity" vs at the drag handle?

@modemmute
Copy link

modemmute commented Dec 14, 2018

Different variation on this same issue, I think.

I have very tall draggables in a scrollable container div (overflow: auto). If you scroll down to a point where the center point of the draggable is above the top of the container div, the draggable fails as if you've dragged it outside of its droppable area.

Suggested solution (similar to @gihrig) would be to offer the possibility to use the grab point as the place which determines the position of the draggable, instead of just presuming the center.

Another thought - could react-virtualized be used as a solution ( #68 )? If we're only rendering a shorter draggable (because react-virtualized isn't rendering what's outside of the container) then the center would always be inside of the droppable.

Great library tho - really enjoying using it.

@Jannikdam
Copy link

Expected behavior
An element "A" should move up or down across the element that is being dragged,"B", when the (upper or lower) border of B crosses the center of A.
This would be opposite of how draggables moved, before version 10:

A resting drag item will move out of the way of a dragging item when the centre position of the dragging item goes over the edge of the resting item. Put another way: once the centre position of an item (A) goes over the edge of another item (B), B moves out of the way.

So, instead of letting the break point be: Center of dragged item hits edge of resting item (meaning a large item could cover several smaller items before they reach the center),
I suggest the break point is set to: Edge of dragged item hits center of resting item.
With this behavior resting items would never be covered more than 50% by the item being dragged, no matter their size. Items would also land as close as possible to where they are dropped, which currently is not always the case.
In a list with elements of equal size the behavior would be similar to what it is now, and in lists with different sized elements (such as the demo above) the behavior should be better..

Actual behavior
The current logic (in v10) gives behavior like this
long_drag

Smaller elements slide over bigger ones immediately, it would be more natural that they snap across the element when they are dragged half way over.
short_drag

Thanks a lot for the library.

Originally posted by @Jannikdam in #517 (comment)

@vpillinger
Copy link

vpillinger commented Feb 22, 2019

Just want to add my two cents that the center drag position makes this library unusable with larger elements. Additionally, in my opinion the center drag point is not good on smaller elements either.

The fundamental problem is that a user isn't going to be mentally calculating the center of the object in their head. A user's mental focus is on the cursor position and the destination position, not the surrounding object. Therefore, even with a medium sized object they are going to be moving their mouse over the other items and wondering why the thing doesn't move. If you don't believe me, show some of the examples that have been posted to random people and see if any of them find it easy to use.

Here is an article that goes over drag and drop designs/styles. Note that in every one of these examples, the drag is based on the cursor position: https://uxdesign.cc/drag-and-drop-for-design-systems-8d40502eb26d

Lastly, in terms of physicality, the center drop point is actually a negative aspect. In the real world, objects move WHERE WE GRAB THEM; unless an object is exceedingly heavy that we must grab it from the center to control it properly. This can be evidenced by picking up a pen at its edge. Unless you intentionally hold it very lightly, it will simply follow along at whatever angle you picked it up at. Its center of gravity simply is not that important relative to its overall weight.
Following that logic, a center drag position gives screen elements an "heavy" feel which makes reordering seem burdensome. This heavy feeling may be useful if you want to recreate the feeling of tossing boulders, but I personally don't want my rearranging my to-do list to feel like a workout routine. I would much rather it feel like I am rearranging light things.

To address the concern that a user can still end up in a situation where they grabbed in bad place and cannot drop, this is certainly a possibility. However,

  1. The user is focusing on their cursor position, so they will notice immediately what the issue is and be able to correct it. With the center drop position, they have to mentally calculate the center of the dragging object and finagle it into place.
  2. If the drop location is on the screen, then the user can reach it with the cursor, but they may not be able to reach it with the center point of the object.

In conclusion, the reason that the center drag position "works" on small objects is mainly a coincidence that the distance between the cursor and the center point is small, but it is still a less than optimal solution.

@artooras
Copy link

I think both approaches have their pros and cons. If a prop could be introduced to use the grab point for the calculation as opposed to the centre point (default), I think this would be the solution that satisfies all use cases.

@alexreardon
Copy link
Collaborator

Something like that could be the path forward @artooras

@artooras
Copy link

artooras commented Mar 4, 2019

@alexreardon, any idea of an ETA for this update? Is it in the plans?

@TF123456
Copy link

TF123456 commented May 5, 2019

In the interim are there any quick workarounds for this? Could the draggable always be assumed to be a certain height?

@vpillinger
Copy link

@TF123456 What we are doing as a quick hack is collapsing our elements before the dragging starts so all of the elements are a reasonable size.

We plan on updating this repo and making a PR at some point, but its a low priority item for us.

@TF123456
Copy link

@vpillinger please could you give a summary of the hack? I have tried this myself but the dragging centre remained for the larger size.

@pdrbrnd
Copy link

pdrbrnd commented May 16, 2019

@TF123456 we also have a dirty hack to achieve the resize prior to drag. We basically hijack dragHandleProps to add some custom functionality.
Something like

{
  ...dragHandleProps,
  onMouseDown: (...args) => {
    this.handleBeforeDrag()
    dragHandleProps.onMouseDown(...args)
  }
}

our problem is with touch devices. using this same approach with onTouchStart makes everything conflict with the page scroll and behave overall strangely.

@vpillinger have you guys managed to make this work with touch devices?

@vpillinger
Copy link

vpillinger commented May 16, 2019

@pbrandone We did not have mobile as a use-case in this specific scenario, so we just ignored it.

@pdrbrnd
Copy link

pdrbrnd commented May 16, 2019

@alexreardon is there any ETA on the introduction of a prop to use the grab point for the calculation as opposed to the centre point?

@crapthings
Copy link

same here

https://streamable.com/cy9xs

@codenamerhubarb
Copy link

I'd also love to see a solution to this.

@kylecrossman
Copy link

For anyone looking for a simple (ish) workaround in the mean time like I was yesterday, here's what I was able to come up with if you have a drag handle:

  1. I made my "Draggable" a full class component that I simply passed in the provided and snapshot objects as props. I don't think this would work with a functional component. That said, in my case at least, you can monkey patch the onMouseDown event handler on your drag handle to set state that will tell your component it is being dragged:
onMouseDown={ event => {
  this.setState({
    dragging: true
  });

  this.props.provided.dragHandleProps.onMouseDown(event);
}}

I believe this works because onMouseDown fires and updates the component before snapshot.isDragging becomes true (which I tried leveraging but the height calculation had already taken place). You can then use that state to detect dragging and adjust the size of the DOM node to a size that you would like to use for your draggable item. So for example, my list had items with generally a height of 28px but allowed for variable heights, so I added a class .is-dragging to set the height to 28px and cut off overflow. This made the drag and drop work as expected.

  1. I added a componentDidUpdate to kill off the dragging state since the dragHandle doesn't seem to have any event listeners to capture this:
componentDidUpdate(prevProps) {
  if (prevProps.snapshot.isDragging && !this.props.snapshot.isDragging) {
    this.setState({
      dragging: false
    });
  }
}

I haven't been able to test this too much yet but it appears to work for now until the library can be updated.

@keywinf
Copy link

keywinf commented Oct 16, 2019

Thanks @crosszilla! I'll try this one very soon, didn't think setting the state at the Draggable level could work when it does not higher in the react dom. I found other dirty solutions, but got a big lag at the end, so I won't hesitate to try yours, which sounds very interesting!

@MYKEU
Copy link

MYKEU commented Oct 25, 2019

Running into the same problem as others - hopefully this will be fixed soon or in v12?

@alexreardon
Copy link
Collaborator

Not in the initial 12. But I hope to do this in a minor release

@ZYinMD
Copy link

ZYinMD commented Nov 5, 2019

It's funny I experienced the same issue but horizontally: I want to drag an item from a fat list to a narrow list, but can't, because I can never drag past its center.

An option to either use the grab point or the drag-handle as the control point would be nice. I already have a small drag-handle.

( ↓ the left list is actually not a list, but 3 small droppables, but you get the idea:

demo

@castiedemann
Copy link

castiedemann commented Nov 12, 2019

This is a big issue for us at the moment, we make use of lots of lists with mixed size items in our application.

Currently in get-reorder-impact.js, it looks like the algorithm finds the closest item based on the whether the center of the dragged item has crossed over into the start-end bounds of other items in the list. This works ok for medium to small items of the same size, but is unnatural when moving large items amongst small items. The logic also takes into account the movement direction which can lead to weird behaviour with multiple items flipping back and forth as the movement direction changes.

bad

I've taken a stab at modifying the reorder logic. This modified approach seems to work well for our needs, though not sure if it covers all of the edge cases, and is currently not working with combine enabled which isn't something we need.

// Determine the current index of the target within the list.
// This will either be the index of the previous impact destination or if
// there hasn't been a previous impact yet fallback to the initial index
// of the draggable.
const prevDest =
  previousImpact && previousImpact.at && previousImpact.at.destination;
const initialIndex = draggable.descriptor.index;
const targetIndex: number = prevDest ? prevDest.index : initialIndex;

// Calculate the current start and end positions of the target item.
const offset = displacement / 2;
const targetStart = targetCenter - offset;
const targetEnd = targetCenter + offset;

// Initialize the newIndex to the current index of the target.
// If no updated index is found in the loop below then the `newIndex` will
// remain unchanged i.e. maintaining the current index of the target item.
let newIndex = targetIndex;

// Loop through all items in the list excluding the current target item
for (let i = 0; i < withoutDragging.length; i++) {
  const child = withoutDragging[i];

  // The `withoutDragging` list does not include the target item, so for all
  // elements after the target in the list shift the index forward by 1
  const listIndex = i >= targetIndex ? i + 1 : i;

  // Determine whether the child has been reordered, i.e. whether the child
  // is currently ordered before the target in the list but was initially after,
  // or is currently ordered after the target and started before.
  const startedAfter = getDidStartAfterCritical(
    child.descriptor.id,
    afterCritical,
  );
  const isAfter = listIndex > targetIndex;
  const hasReordered = startedAfter !== isAfter;

  // If the child has been reordered then the center position will have been
  // shifted back/forward by the displacement size of the target item.
  // To get the current center position of the child item adding or subtract
  // the displacement size from the center position based on whether the target
  // was initially before or after the item.
  const childDisplacement = startedAfter ? -displacement : displacement;
  const boxCenter = child.page.borderBox.center[axis.line];
  const center = boxCenter + (hasReordered ? childDisplacement : 0);

  // Check if the target item belongs at the child items index based on whether:
  // A: The target start position is less than the center of the child and the
  // child is before the target in the list.
  // or
  // B: The target end position is greater than the center of the child and the
  // child is after the target in the list.
  if ((targetStart < center && !isAfter) || (targetEnd > center && isAfter)) {
    newIndex = listIndex;
    break;
  }
}

And this also requires passing down the previousImpact to the getReorderImpact function.

Here's that in action:

better

Basically this kind of inverts the logic, so that instead of finding if the center of the dragged item is within another item, we shift to a particular index based on whether the start/end edge of the dragged item has crossed beyond the center of neighbouring items.

@alexreardon is there any ETA on when we might be able to have this issue looked into? I'd be happy to submit a PR with my proposed fix as it is now, though like I said it doesn't currently cater for collapse mode. If I find time I can look into that.

@alexreardon
Copy link
Collaborator

Thanks for putting forward this @CasperSmith! It looks promising for sure. To start with, can you please add more comments to your solution explaining the logic going on. This will make it easier for me to better understand the overall approach you are taking

@castiedemann
Copy link

@alexreardon thanks for the quick response! I've updated the snippet in my original post with more comments. Let me know if you need any more clarification

@alexreardon
Copy link
Collaborator

alexreardon commented Nov 13, 2019

Basically this kind of inverts the logic, so that instead of finding if the center of the dragged item is within another item, we shift to a particular index based on whether the start/end edge of the dragged item has crossed beyond the center of neighbouring items.

(Emphasis added to help me understand)

This is a fantastic suggestion that I plan on whiteboarding to see how it holds up in various scenarios + with combining.

Note to others: this strategy is an improvement for reordering items with drastically different sizes and not for moving large items into smaller drop zones (that is a seperate thing)

@CasperSmith Perhaps you could open a PR with your current implementation so we can play with it on the netlify build. All good if the tests / types / linting fails - it is just to get a feel for the algorithm

@castiedemann
Copy link

castiedemann commented Nov 13, 2019

@alexreardon sure 👍

@jacobwicks
Copy link

jacobwicks commented Nov 13, 2019

If anyone is having a problem dragging wide draggables into droppables, this code will somewhat address the issue. I based it on the code that valentinvoilean posted in the sandbox link and the function that jeroenvervaeke posted. Thanks to both of you!

On mouseDown it changes the drag width of the draggable item to 50 pixels and centers it under the cursor. This means that the 50% line will be located very close to the cursor, which will give intuitive dragging over results in my use case, which is a wide draggable and a wide but not very tall droppable. It has some drawbacks, notably that a wide item grabbed near the right end will visually "jump" to the right, but at least in my case the ease of dropping is more important than maintaining visual consistency.

It might be possible to keep the visual representation of the item in the same place on the screen while still making the draggable 50 px wide and centered on the cursor, but I'm not going to figure that part out unless I get feedback from my users that I need to.

if you want to see the numbers involved, you can console.log(event.nativeEvent, boundingRect);

`
const DRAG_SIZE = 50;

onMouseDown={(event: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
const target = event.currentTarget;
const boundingRect = target.getBoundingClientRect();

           const targetX = event.nativeEvent.clientX - boundingRect.left;
            const translateX = targetX - DRAG_SIZE / 2;
            
            //translateX moves the position of the item on the horizontal x axis
            target.setAttribute('style', 
            `width: ${DRAG_SIZE}px;
            transform: translateX(${translateX}px)`);

          if (provided.dragHandleProps !== null) {
            provided.dragHandleProps.onMouseDown(event);
          }
        }}` 

To undo it on mouseUp where no drag occurred:
` onMouseUp={(event: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
const target = event.currentTarget;
const boundingRect = target.getBoundingClientRect();

          const targetX = event.nativeEvent.clientX - boundingRect.left;
            const translateX = targetX - DRAG_SIZE / 2;
            
          target.setAttribute('style', 
          `width: ${DRAG_SIZE}px;
          transform: translateX(${-translateX}px)`);
        }}`

@alexreardon
Copy link
Collaborator

I am hoping to dive into the problem space very soon

@alexreardon
Copy link
Collaborator

We are actively working on this. We are building on the ideas you put forward @jacobwicks. Thank you so much!

@alexreardon
Copy link
Collaborator

This has shipped in 12.2.0 💃

@MYKEU
Copy link

MYKEU commented Dec 6, 2019

This is definitely working better than previously - however, I have noticed that the problem persists if the item I am dragging has a height greater than my browser's height (unable to scroll up and down whilst dragging). Not sure if others are facing this as well.

@alexreardon
Copy link
Collaborator

alexreardon commented Dec 6, 2019

@mikeyshing88, i would question whether something draggable should be bigger than the browser height. how are they to know where it is going? 🤔

@arvid-backhagen
Copy link

@mikeyshing88, i would question whether something draggable should be bigger than the browser height. how are they to know where it is going? 🤔

I have the same issue, since the users can customize the contents of their items. Our solution currently is to be able to collapse the items. But it would be neat to be able to drag large items without issues as well.

@alexreardon
Copy link
Collaborator

Can a new issue please be created with a stand-alone example? It would be helpful to figure out what we can do.

@gihrig
Copy link

gihrig commented Dec 6, 2019

@alexreardon Like @arvidlarzzon said, I have objects containing user input that can potentially grow to many times screen height. My solution is to allow dragging only by a drag handle at the top of the object. In the past, I tried collapsing the object on drag but this didn't work. I have not visited that code in a while, but as long as the draggable triggers the drop target by the location of the mouse pointer it should not matter how large the draggable object is.

@alexreardon
Copy link
Collaborator

You could now collapse it in onBeforeCapture - but feel free to create a new issue and we can look into that specific usage case

@ghost
Copy link

ghost commented Apr 16, 2020

This has shipped in 12.2.0 💃

@alexreardon I can't find any documentation for how to control the max size.
Can any one help?

@JamesGelok
Copy link

JamesGelok commented Aug 19, 2020

@muhammedmagdi I'm also having some trouble finding documentation on how to control it. Did you figure it out?

edit:
Nevermind, found their blog post about it.
Found it by looking at the 12.2.0 release notes.
Leaving changes here if anyone else finds this thread. (Hi, Internet!)

in other words, make sure your version is >= 12.2.0

@JoeVanGundy
Copy link

Hi there, I'm still a little confused after reading everything.
Is it possible to use the drag handle as the center point of the Draggable?

@lucasmonstrox
Copy link

News? I cant find any way on current docs :s

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

No branches or pull requests