atlassian / pragmatic-drag-and-drop

Fast drag and drop for any experience on any tech stack
https://atlassian.design/components/pragmatic-drag-and-drop
Other
9.77k stars 223 forks source link

Request to support optionally passing `window` as a param #123

Open jordanwilliams opened 2 months ago

jordanwilliams commented 2 months ago

👋🏾 Hello again! After getting around the react-dnd issue I described in https://github.com/atlassian/pragmatic-drag-and-drop/issues/106, I was able to use pdnd to support drag and drop in some feature code here at Slack 🚀 . Overall it was pretty smooth, though I'm reaching out with a request that will help me release the change. The request is to optionally support passing window as a param.

For context, the Slack desktop client supports multiple windows. Drag and drop works great in our "main" window, though it doesn't work if you open a view in a separate "child" window. We manage child windows from the main window, and we cannot access window or window.document directly. We instead pull the window from React context (e.g. const win = useWindow()) to ensure we're using the correct window regardless of where a view is rendered. There are some uses of window/document inside of pdnd that I'm hoping can be updated to use a provided window param instead if it is passed. For example, react-dnd supports passing the window as the context param to their DndProvider (source). Another example would be downshift which supports passing the window as an environment param (docs).

// An example of our current usage of react-dnd
function ExampleComponent() {
    const win = useWindow();

    return (
        <DndProvider backend={HTML5Backend} context={win}>
            {children}
        </DndProvider>
    );
}

Below is an example of how this could potentially look in pdnd

// An example of passing window to pdnd in the future
function ExampleComponent() {
    const ref = useRef<HTMLDivElement | null>(null);
    const win = useWindow();

    useEffect(() => {
        const element = ref.current;
        if (!element) return;

        return combine(
            draggable({
                element,
                environment: win,
                // ... other params
            }),
            dropTargetForElements({
                element,
                environment: win,
                // ... other params
            }),
        );
    }, [win]);

    return <div ref={ref}>Drag me</div>;
}

I've only briefly looked at the pdnd source code and saw that there might be a chance to thread it through draggable to adapater.registerUsage and ultimately to mount. But I don't want to oversimplify the changes, as I'm not aware of all the places that access window or document. Let me know if this is something you all would be open to! It would be a massive help to us in our effort to adopt pdnd here at Slack.

alexreardon commented 1 month ago

Relevant thread: https://github.com/atlassian/pragmatic-drag-and-drop/issues/24#issuecomment-2290171561

Right now pdnd requires that each window manages it's own elements. In order to support the full breadth of the drag and drop API (text selection, external drags, draggable elements) I don't see a reasonable path to also supporting reaching into windows and managing drags from inside there for all drag types.

For elements specifically there might be a path - but it would likely introduce a lot of complexity into pdnd 🤔

jordanwilliams commented 1 month ago

@alexreardon thanks for linking to that other comment. I checked out the Seamless drag and drop between applications video you linked to in that comment and it was fantastic.

I appreciate the consideration and can understand that there isn't a reasonable path to support the request. While I've only leveraged the "element" adapter so far, we'd definitely want to make use of the "external" adapter as well to support dragging items between windows. Shortly after I had submitted this issue, I explored trying to patch pdnd to work for us. I was able to thread a window param into some parts of the element adapter without much fuss, though there were other parts that I couldn't quite figure out. There were several instanceof HTMLElement checks that needed to be updated. I didn't pursue this any further.

I'm at a bit of a crossroads. I'm very keen on using pdnd. You sold me on it through the videos, the API has been really pleasant to work with, and it would unlock some experiences for us (dragging between windows and applications) that we can't do with our current libraries. But our approach of having the main window manage child window content is at odds with the pdnd requirements. It's also unlikely to change because there are some state management benefits we're receiving. I've been chatting with some folks internally to see if we can come up with some solutions on our end so that we can use pdnd as-is. I'll let you know where we end up with this. That being said, feel free to close this issue. Thank you so much for all your help!

michaelabrahamian commented 2 days ago

👋 Hey @jordanwilliams! Were you able to find a solution that works for you, using the current API?

jordanwilliams commented 20 hours ago

@michaelabrahamian unfortunately I wasn't able to! I would still love to use pdnd, but without a way to specify which window to use, we can't adopt it. We're continuing with react-dnd in the meantime. If anything changes down the road, I'd be happy to be a guinea pig. Otherwise I can update you if something changes on our end.