Better API for rich components

[ChocolateLoverRaj]

The props injected into child components can conflict with the other props of the child component. Example:

import { forwardRef, useRef } from "react";
import Draggable from "react-draggable";
import { italic } from "./App.module.css";

const Component1 = forwardRef((props, ref) => {
  const { className } = props;

  console.log(props);

  return (
    <div {...props} ref={ref}>
      <span>This should not be italic</span>
      <br />
      <span className={className}>This should be italic</span>
    </div>
  );
});

export default function App() {
  const ref = useRef(null);

  return (
    <Draggable nodeRef={ref}>
      <Component1 ref={ref} className={italic} />
    </Draggable>
  );
}

Link to codesandbox: https://codesandbox.io/s/react-draggable-bug-3-p99yd?file=/src/App.js

In this example, creating <div {...props} /> results in the div getting unwanted props.

There should be a better API for components like Component1, preferably without modifying the props. This could be done by lifting the state up into the component that renders <Draggable /> and <Component1 />. This is what a 'better API' could look like:

import { forwardRef, useRef } from "react";
import Draggable, { useRichComponent } from "react-draggable";
import { italic } from "./App.module.css";

const Component1 = forwardRef((props, ref) => {
  const { draggableProps, className } = props;

  return (
    <div {...draggableProps} ref={ref}>
      <span>This should not be italic</span>
      <br />
      <span className={className}>This should be italic</span>
    </div>
  );
});

export default function App() {
  const ref = useRef(null);
  const { childProps, internalControl } = useRichComponent();

  return (
    <Draggable nodeRef={ref} internalControl={internalControl}>
      <Component1 ref={ref} className={italic} draggableProps={childProps} />
    </Draggable>
  );
}

The above example uses a hook for the <Draggable /> to control the props of <Component1 /> without modifying the jsx element props. This is similar to Form.useForm() in Ant Design.

This issue is related to #585 because I don't think the solution to the issue is a good API, as it can cause problems demonstrated in the sandbox.

[STRML]

What does internalControl do? And why not just do this:

const Component1 = forwardRef((props, ref) => {
  const { className, ...restProps } = props;

  console.log(props);

  return (
    <div {...restProps} ref={ref}>
      <span>This should not be italic</span>
      <br />
      <span className={className}>This should be italic</span>
    </div>
  );
});

[ChocolateLoverRaj]

@STRML internalControl is some function / object that allows the draggable component to control the value of childProps. Internally, it could be using useState to get and set the props.

[ChocolateLoverRaj]

@STRML your example would not work because then the draggable class name won't be on the div. The component would need to extract the draggable class names from the custom class names, and put the draggable class names in the div and the custom class name on the 2nd span. I'm not sure of the class name prop is necessary for the dragging to work, but I'm assuming not having the class name on the div can break things.

[STRML]

It doesn't need the class.

Another way we could work around this would be to set the event handlers on the ref instead of the child, so the props don't even need to be passed.

On Wed, Jul 21, 2021 at 10:25 PM Rajas Paranjpe @.***> wrote:

@STRML https://github.com/STRML your example would not work because then the draggable class name won't be on the div. The component would need to extract the draggable class names from the custom class names, and put the draggable class names in the div and the custom class name on the 2nd span. I'm not sure of the class name prop is necessary for the dragging to work, but I'm assuming not having the class name on the div can break things.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/react-grid-layout/react-draggable/issues/586#issuecomment-884616154, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJEKPZD6JPGED5GJZXPVW3TY56SNANCNFSM5AY2FDKA .

[ChocolateLoverRaj]

@STRML I think that's the best solution.

[STRML]

There's going to be some performance loss as a result of this unfortunately, as event handlers would be directly attached to every single Draggable rather than delegated by React. For simple use cases this is fine, but for larger use cases like React-Grid-Layout (where there may be hundreds of draggables between the grid items & the resize handles), I am not sure this is an appropriate tradeoff.

[ChocolateLoverRaj]

@STRML then I think my first solution would work best, with the useRichComponent hook. That would let React do the event handler stuff while still being flexible.

[ozydingo]

I am running into this as well. My component's inner HTML element is given many props, some of which conflict with those injected by Draggable. My custom component has props that are not appropriate to pass to the inner HTML element. Therefore, I've landed on the following good-enough solution to define a helper function to merge the props I actually want to pass to my inner HTML element with those that are required to allow Draggable to function:

// Merge in props passed from Draggable and DraggableCore into desired props
function mergeDraggableProps(receivedProps, desiredProps) {
  return {
    ...desiredProps,
    className: cn(receivedProps.className, desiredProps.className),
    style: {
      ...receivedProps.style,
      ...desiredProps.style,
    },
    transform: receivedProps.transform, // NOTE: I don't have any transform so didn't bother merging here
    onMouseDown: receivedProps.onMouseDown,
    onMouseUp: receivedProps.onMouseUp,
    onTouchEnd: receivedProps.onTouchEnd,
  };
}

const MyComponent = forwardRef((props, ref) => {
  // some props are not appropriate to forward blindly to the `div` element;
  // they are stripped out in the `mergeDraggableProps` function and named here for illustration.
  const { foo, bar, notDivProps } = props

  // ...

  // simplified example covering some of the conflicting props and a couple additional ones.
  const targetProps = {
    className: 'foo bar',
    style: {
      left: `${left}rem`,
      width: `${width}rem`,
    },
    onClick: noSeek,
    tabIndex: 0,
  };

  const innerProps = mergeDraggableProps(props, targetProps);

  return (
    <div {...innerProps} ref={ref}>foo</div>
  );
}

I think a good solution would be if Draggable could handle this merging instead of clobbering props including className and style. And/or provide a merge function such as the one I wrote above, as a part of the library's domain instead of app domain.

[ozydingo]

I just wrestled with getting this fully hooked up with TypeScript, so worth documenting here:

function hasDraggableProps(props: Partial<DraggableChildProps>): props is DraggableChildProps {
  return (
    typeof props.className === 'string' &&
    typeof props.style === 'object' &&
    typeof props.transform === 'string'
  );
}

interface DraggableCoreChildProps {
  onMouseDown: MouseEventHandler<HTMLDivElement>;
  onMouseUp: MouseEventHandler<HTMLDivElement>;
  onTouchEnd: TouchEventHandler<HTMLDivElement>;
}

interface DraggableChildProps extends DraggableCoreChildProps {
  className: string;
  style: React.CSSProperties;
  transform: string;
}

function mergeDraggableProps(
  draggableProps: DraggableChildProps | Record<string, unknown>,
  childProps: Partial<DraggableChildProps> & Record<string, unknown>
) {
  if (!hasDraggableProps(draggableProps)) {
    return childProps;
  }

  return {
    ...childProps,
    className: cn(draggableProps.className, childProps.className),
    style: {
      ...draggableProps.style,
      ...childProps.style,
    },
    transform: draggableProps.transform,
    onMouseDown: draggableProps.onMouseDown,
    onMouseUp: draggableProps.onMouseUp,
    onTouchEnd: draggableProps.onTouchEnd,
  };
}

Note that this solution doesn't account for DraggableCore usage without Draggable, but it would be easy to extend it to do so.

Note also that if imported into the Draggable lib, it could be somewhat simplified since hasDraggableProps wouldn't be necessary. It is in application core since we need to allow for the component to be initialized without the draggable props as well as with the draggable props.