konvajs / react-konva

React + Canvas = Love. JavaScript library for drawing complex canvas graphics using React.
https://konvajs.github.io/docs/react/
MIT License
5.76k stars 256 forks source link

Draggable components switch between 'controlled' and 'uncontrolled' #360

Closed kmannislands closed 4 years ago

kmannislands commented 5 years ago

The API for draggable components violates react's controlled vs uncontrolled component paradigm. Any component marked draggable seems to switch between managing its own state and respecting the x, y values that it gets as props.

In my use-case, react-konva is the renderer for a physics-backed layout. When users drag a Rect, there is some elastic feedback, so the Rect shouldn't be right under the cursor but instead lags behind it by an increasing amount as it is pulled. Since the components switches between the x, y value that it is getting as props (correct, from my physics simulation) and its own x, y state that is is managing since it is draggable, there is a 'twitching' effect in the graphics.

Here's a GIF of the behavior: twitchy-graph

As a much more contrived but direct minimal repro, consider this codePen: https://43momq7017.codesandbox.io/

It should be fairly easy to get the twitching effect I described when you drag the Rect as the components switched between controlled and uncontrolled.

lavrton commented 5 years ago

I don't get it. What do you suggest?

kmannislands commented 5 years ago

@lavrton there should probably be two explicit API's, controlled an uncontrolled.

Usually, uncontrolled components will take an 'initial value' as a prop. So in this case, initial position:

// Manages its own state. Props tell it where is starts. It manages its state after that
<Rect
  draggable
  initialX={20}
  initialY={20}
/>

The current API is basically that of a typical controlled component:

<Rect
  x={this.state.x}
  y={this.state.y}
  // If this didn't set state, the Rect would never move:
  onDragMove={({ evt: { x, y }}) => {
    this.setState({ x, y });
  }}
/>

A quick google shows that other react draggable libraries follow this pattern as expected. Interestingly, the library in the example I linked also lets users drag when it is controlled (for convenience I'm guessing) but they recognize that this violates react's philosophy and they allow this behavior to be disabled. This would be a reasonable solution for this library as well, especially considering backward compatibility.

The current API is inconsistent if you think about it. If a component has props draggable: true as well as x, y which are updated onDragEnd, how does the component decide what its position is between drag start and drag end?

Ie, how does it choose to respect its internal x ,y position from dragging over the x, y that are being provided as props? This is the inconsistency.

seankimdesign commented 5 years ago

I've noticed this issue as well. Dragged elements seems to maintain an internal state outside of the React paradigm. I think react-konva version 1.7.9 and below actually does not have this problem. It must have been introduced short time after.

kmannislands commented 5 years ago

Looking at tickets, it appears you are already somewhat across this one, @lavrton:

By your own words from https://github.com/konvajs/react-konva/issues/256:

If we have a shape with draggable = true it can be dragged by a user. So its position (x and y) will be changed. But that information will be not saved back to component state (so it still equals to {x: 10, y: 10}). I don't see any valid use cases for such behavior.

We should handle a position change of a node and save it back to the state. For that purpose, we can use dragmove or dragend events. It depends on the use cases and on the app behavior which event to use.

The way that you support both the dragmove and dragend-type use-cases via React's paradigm is to provide both controlled and uncontrolled API's (as described above) respectively.

lavrton commented 5 years ago

Yeah, there were some related changed called strict-mode https://github.com/konvajs/react-konva/#strict-mode.

We can implement uncontrolled API with defaultX and defaultY. Not sure how it affects the performance, but it will be simple.

But for now, I don't know how to make fully controlled drag&drop. I don't see good ways to "cancel" or fully control the position of the node from react-konva.

seankimdesign commented 5 years ago

Enabling the strict mode worked for my use case. I can finally upgrade React to the newest versions. Thank you.

Vicfeel commented 5 years ago

Yeah, there were some related changed called strict-mode https://github.com/konvajs/react-konva/#strict-mode.

We can implement uncontrolled API with defaultX and defaultY. Not sure how it affects the performance, but it will be simple.

But for now, I don't know how to make fully controlled drag&drop. I don't see good ways to "cancel" or fully control the position of the node from react-konva.

@lavrton this method works for me, but there has no exported member 'useStrictMode' in react-konva.d.ts, i got typescript error.

version: react-konva@16.8.7-3

lavrton commented 5 years ago

Right, want to make a Pull Request?

Vicfeel commented 5 years ago

Right, want to make a Pull Request?

https://github.com/konvajs/react-konva/pull/379

lavrton commented 4 years ago

I was thinking about this issue. To make a fully controlled component you can do this:

onDragMove={e => {
  const newPos = e.target.position();
  // reset position to its old state
  // so drag is fully controlled by react
  e.target.position({ x: pos.x, y: pos.y });

  // your set state
  setPos({
    x: Math.min(100, newPos.x),
    y: Math.min(100, newPos.y)
  });
}}

The idea is to reset the node position to its original value that we have in the react. And then use new position to set a new state. If you comment setPos the node will be not movable. So it works as a controlled component.

Demo: https://codesandbox.io/s/react-konva-controlled-drag-demo-pxmg8

Probably that little logic in onDragMove can be abstracted into something like react hook.

sfeidersullivan commented 1 year ago

Hi @lavrton, thanks for the great library!

This strategy of reseting the position works well when you know the position of the target, but what about for Groups where the groups position isn't tracked in state?

Here's a demo: https://codesandbox.io/s/kind-ptolemy-kmgvs2?file=/src/App.js

Setting the Groups position to { x: 0, y: 0 } then transforming each child's position by the drag event movement seems to work, but when trying to implement drag boundaries on the entire Group, getAbsolutePosition() and getClientRec() don't behave as expected. Any ideas of what I am missing here?

Edit: To clarify, my expectation would be that getClientRec returns the position of the bounding box with respect to the Layer origin (so that I can tell if any part of the Group has overflowed).

lavrton commented 1 year ago

@sfeidersullivan I don't like the approach with reset position of dragging group. Instead I would:

  1. Reset position only on dragend. Knowing that position is changing, while I drag the group
  2. Instead of draggable group, just make all shapes draggable. When you start drag a line, you can manually start drag of circles. Something like this: https://codesandbox.io/s/inspiring-butterfly-26w42m?file=/src/App.js
sfeidersullivan commented 1 year ago

Thanks @lavrton!

  1. This approach should work well (assuming nothing outside the group needs to be re-rendered via a state update. e.g. a line from a fixed point to the group).
  2. Looks like your Codesandbox link is broken 😐.
lavrton commented 1 year ago

Ah, sorry. It was private. I made it public.