xplato / useUndoable

↪ React hook for undo/redo functionality (with batteries included)
MIT License
168 stars 7 forks source link

Update react flow example with the react flow renderer version 10 #3

Closed carlosporta closed 2 years ago

carlosporta commented 2 years ago

Hello,

The example on this repository is using react flow renderer version 9. Could you update the example to use the react flow renderer version 10?

xplato commented 2 years ago

Great idea! Thanks for telling me.

v10 has a fundamentally different state implementation, so it may take a little while to get everything working properly.

Essentially, the only thing you really need to do is split the useUndoable state into an object with separate arrays for nodes and edges.

useUndoable({
    nodes: initialNodes,
    edges: initialEdges,
});

The problem I am running into is the onNodesChange function. It will change the state after literally every pixel move. This means that when you undo after moving a node, it will start by undoing each individual pixel you move. The only solution to this problem is the onNodeDragStop function, but that doesn't seem to play nicely with a custom state object.

I'll update the example to v10 when I can. Thanks again for letting me know!

carlosporta commented 2 years ago

Yes, I am facing the same situation. To avoid every pixel move, I am filtering the changes on onNodeChangeCallback

const change = changes[0]
if (change.type === "position" && change.dragging) {
  return;
}
else {
  // update state
}
carlosporta commented 2 years ago

I am also having trouble when I try to delete a node. When I try to delete a node two events are fired one after the other. First, a remove event is fired followed by a select event. The remove event removes the node, but the select event recreates it again. For now, I am filtering the select event, like this:

const change = changes[0];
if (change.type === 'select'){
  return
}
xplato commented 2 years ago

I am having the same issue, actually.

Could you share some more of your code?

carlosporta commented 2 years ago

I forgot to mention the principal thing. I am managing the state and the historic separately. I am still using useState for nodes and edges, and for the history I am using useUndoable. I still do not have a working example. But if helps this is what I get so far.

const handleHistory = (changes: NodeChange[]) => {
  const change = changes[0];
  if (
    change.type === "select" ||
    (change.type === "position" && change.dragging)
  ) {
    return;
  }

  setHistory((oldElements: Elements) => {
    return {
      // we need to get current updated nodes
      // maybe we can pass the new nodes
      nodes: []
      edges: oldElements.edges,
    };
  });
};

const onNodesChange = (changes: NodeChange[]) => {
  // current code
  handleHistory(changes);
  setNodes((oldNodes) => applyNodeChanges(changes, oldNodes));

  // future code
  // Need to test passing the new nodes
  // setNodes(oldNodes => {
  //   const newNodes = applyNodeChanges(changes, oldNodes);
  //   handleHistory(newNodes);
  //   return newNodes}
  //   );
};
xplato commented 2 years ago

In this particular case, using multiple state values can easily get tricky; keeping them in sync can lead to some weird bugs.

There's really no need to manage them separately. If you only want to use the history internally, just don't give users the option to undo. You can access history, as you might know, by using the future or past variables that the hook gives you.

The main thing to note, and why you're having a problem, is that useUndoable is not primarily a history tracker: its primary purpose is to manage state, keeping a record of which is only a feature.

The reason we are both getting issues with v10 is because we are trying to manage multiple state instances separately.

The only solution is to make useUndoable the exclusive manager of state.

For more information, see the updated react-flow-example and the README for more information about v10.

Thanks again for opening an issue!

carlosporta commented 2 years ago

I am also having trouble when I try to delete a node. When I try to delete a node two events are fired one after the other. First, a remove event is fired followed by a select event. The remove event removes the node, but the select event recreates it again. For now, I am filtering the select event, like this:

const change = changes[0];
if (change.type === 'select'){
  return
}

@xplato, what about this issue? Have you found a solution? I am not able to delete any nodes.

xplato commented 2 years ago

@carlosporta Are you filtering the change.type from the node data directly? Is this an accurate way to know what changes were made?

From React Flow's documentation, it doesn't look like the onNodesChange function exports some identifier related to the actual action.

The best solution would probably be to create a new reducer and find a way to dispatch actions from onNodesChange instead of piping the result directly into state.

For instance, instead of

const onNodesChange = useCallback(
    (changes) => {
        triggerUpdate('nodes', applyNodeChanges(changes, elements.nodes));
    },
    [triggerUpdate, elements.nodes]
);

you'd need to find a way to specify specific actions and dispatch them explicitly:

const onNodesChange = useCallback(
    (changes, actionType) => {
        dispatch({ type: actionType, payload: applyNodeChanges(changes, elements.nodes) });
    },
    [triggerUpdate, elements.nodes]
);

You would then, of course, manually modify the state for each specific action.


As for the issue where moving a node will add tons of new re-renders for each pixel, you could reduce this by changing the state representation onNodeDragStart and then reset it onNodeDragStop.

<ReactFlow
    nodes={hasUserStartedDragging ? stateOfNodesBeforeDragStart : elements.nodes}
/>

The react-flow-example is intended to be a simple example demonstrating useUndoable integration with React Flow. Since useUndoable itself is working as expected, all of these little quirks need to be solved on a project-by-project basis.

This means that the way you determine which actions to dispatch is up to you. To be honest, I'm not sure exactly how to differentiate between different actions. You could try opening an issue in React Flow and asking if adding the change type to the onNodesChange, onEdgesChange functions. That is, instead of just the changes, you also pass a string ID of the action like so:

type Change = 'move' | 'delete' | 'select'; // ... and so on
const onNodesChange = (changes: Node[], changeType: Change) => { ... }

If you have any issues implementing what I've suggested here, feel free to reach out.

honocoder commented 2 years ago

@carlosporta @xplato Hello guys.

First of all, thank you very much @xplato for your work on useUndoable, it's awesome!

I saw you both have an issue that I'm facing now : the "pixel by pixel" undo when moving a node. Have you found any solution for this problem ?

Thanks in advance!

carlosporta commented 2 years ago

Hello, @jimymltta!

I haven't tested yet, have you tried the solution proposed by @xplato?

As for the issue where moving a node will add tons of new re-renders for each pixel, you could reduce this by changing the state representation onNodeDragStart and then reset it onNodeDragStop

<ReactFlow
    nodes={hasUserStartedDragging ? stateOfNodesBeforeDragStart : elements.nodes}
/>
xplato commented 2 years ago

@jimymltta Thank you for your kind words!

As @carlosporta pointed out, I made a suggestion on how to avoid this, but I'm not sure if it will work consistently. Really, the ideal solution would be if the onNodesChange function could ignore move events, so that you could hook those up manually.

I really hope React Flow later finds a way to identify specific events within these catch-all change functions.

I wish you guys the best! Let me know if I can help.

rfdearborn commented 2 years ago

Late here, but - for posterity - one option is to do something like this:

const [undoableLoggingDisabled, setUndoableLoggingDisabled] = useState(false)
...
// adopted from https://github.com/Infinium8/useUndoable/blob/main/react-flow-example/src/App.jsx#L32
const updateElements = useCallback(
  (t: 'nodes' | 'edges', v: Array<any>) => {
    setElements(
      e => ({
        nodes: t === 'nodes' ? v : e.nodes,
        edges: t === 'edges' ? v : e.edges,
      }), 
      null, 
      // @ts-ignore https://github.com/Infinium8/useUndoable/issues/9
      undoableLoggingDisabled
    )
  },
  [setElements, undoableLoggingDisabled]
)
...
const onNodeDragStart = useCallback(
  () => {
    setUndoableLoggingDisabled(true)
  },
  [setUndoableLoggingDisabled]
)

const onNodeDragStop = useCallback(
  () => {
    setUndoableLoggingDisabled(false)
  },
  [setUndoableLoggingDisabled]
)
arthurodriguesbatista commented 2 years ago

does anyone have a working solution for this problem? I have tried the suggestions above but have had no success.

alor commented 2 years ago

the issue with the @rfdearborn solution is that you receive a onNodeDragStart even when a node is selected, so you end up disabling logging forever until a node is dragged (and the onNodeDragStop event is triggered)

using onNodeDrag hook seems to work (not fired on node select, only on dragging) but I have other side effect that I'm still investigating