bvaughn / react-resizable-panels

https://react-resizable-panels.vercel.app/
MIT License
3.99k stars 144 forks source link

onDragging event on PanelResizeHandle does not fire correctly #388

Closed paulm17 closed 3 months ago

paulm17 commented 3 months ago

Similar to: https://github.com/bvaughn/react-resizable-panels/issues/94

Sandbox: [...]

Dragging the handles does not fire the onDragging event. However, if you mouseover the handle, it will.

Could be a regression?

paulm17 commented 3 months ago

Found this sandbox: https://codesandbox.io/p/sandbox/react-resizable-panels-forked-di6vn0?file=%2Fsrc%2FResizeHandle.tsx

In an earlier version, it's working as expected.

paulm17 commented 3 months ago

I ended up forking the project, to see if I can resolve.

By doing so, I found out I didn't need onDragging but also an onDragStart and onDragEnd.

https://github.com/bvaughn/react-resizable-panels/blob/main/packages/react-resizable-panels/src/PanelResizeHandle.ts#L40

I added the following props to PanelResizeHandleProps:

  onDragStart,
  onDragEnd,

But I also fixed the onDrag issue as well. Now fires correctly and not on mouseover/out.

https://github.com/bvaughn/react-resizable-panels/blob/main/packages/react-resizable-panels/src/PanelResizeHandle.ts#L129

const setResizeHandlerState = (
      action: ResizeHandlerAction,
      isActive: boolean,
      event: ResizeEvent
    ) => {
      if (isActive) {
        switch (action) {
          case "down": {
            setState("drag");

            onDragStart?.();

            startDragging(resizeHandleId, event);

            const { onDragging } = callbacksRef.current;
            if (onDragging) {
              onDragging(true);
            }
            break;
          }
          case "move": {
            const { state } = committedValuesRef.current;

            if (state !== "drag") {
              setState("hover");
            } else if (state === "drag") {
              const { onDragging } = callbacksRef.current;
              if (onDragging) {
                onDragging(true);
              }
            }            

            resizeHandler(event);
            break;
          }
          case "up": {
            setState("hover");

            onDragEnd?.();
            stopDragging();

            if (state !== "inactive") {
              const { onDragging } = callbacksRef.current;
              if (onDragging) {
                onDragging(false);
              }
            }
            break;
          }
        }
      } else {
        setState("inactive");
      }
    };
paulm17 commented 3 months ago

Forgot to say. Thank you for creating this library.

Out of all the libraries out there. I've tested all of them...

This one is the best one, bar none!

bvaughn commented 3 months ago

The onDragging handle is supposed to be called once when dragging starts (with a param of true) and once more when it ends (with a param of false). It looks like it's currently calling onDragging(false) too often because it's not checking the previous action. This regressed with #374 so thanks for the bug report. I believe I've fixed the regression in #391.

By doing so, I found out I didn't need onDragging but also an onDragStart and onDragEnd.

onDragStart/onDragEnd sounds like the same behavior as onDragging(true)/onDragging(false), just with a different name 😄 I'll just keep the current name to avoid making backwards-breaking changes.

Forgot to say. Thank you for creating this library.

You're welcome!

bvaughn commented 3 months ago

Bugfix published in version 2.1.1


❤️ → ☕ givebrian.coffee