atlassian/react-beautiful-dnd

Move to portal solution for Draggables

onemenny opened this issue · 20 comments

onemenny commented

Bug or feature request?

Feature request

Well, it is lame, as mentioned in the ReadMe. I had to deal with transitionend event, plus add many patches to get it semi working. I do believe that transitions have become a standard with animations and more. Having a transitioned panel as a parent of beautiful-dnd is not far fetched, as I had to deal with it. Append to body option is something many 3rd party component adds to give a 360 degree compliance

I for one, would certainly like to see this feature added

Any way, thank you for the hard work.

alexreardon commented

For context:

Warning: position: fixed

react-beautiful-dnd uses position: fixed to position the dragging element. This is quite robust and allows for you to have position: relative | absolute | fixed parents. However, unfortunately position:fixed is impacted by transform (such as transform: rotate(10deg);). This means that if you have a transform: * on one of the parents of a Draggable then the positioning logic will be incorrect while dragging. Lame! For most consumers this will not be an issue. We may look into creating a portal solution where we attach the dragging element to the body rather than leave it in place. However, leaving it in place is a really nice experience for everyone. For now we will leave it as is, but feel free to raise an issue if you this is important to you.

Now that react has a supported portal feature we could look into doing it. I am keen to avoid any regressions on the consumption experience which is currently really nice. We will probably not look at this for a while as we have a large number of higher priority features we want to ship. In the mean time you might find this issue helpful: #128 specifically the part about overriding top/left values rather than using the transform.

onemenny commented

Thank, I was aware of your constraints. I will look into it.

kasperpihl commented

I managed to get this to work with Portal from react-dom without changing styles etc.

I added a dom element into my html

<div id="draggable"></div>

Added the following styles

position: absolute;
pointer-events: none;
height: 100%;
width: 100%;

And then added this to the Draggable

import React, { PureComponent } from 'react';
import { createPortal } from 'react-dom';

const _dragEl = document.getElementById('draggable');

class DraggableGoal extends PureComponent {
  optionalPortal(styles, element) {
    if(styles.position === 'fixed') {
      return createPortal(
        element,
        _dragEl,
      );
    }
    return element;
  }
  render() {
    const { item } = this.props;
    return (
      <Draggable>
        {(provided, snapshot) => {
          return (
            <div>
              {this.optionalPortal(provided.draggableStyle, (
                <div
                  ref={provided.innerRef}
                  style={provided.draggableStyle}
                  {...provided.dragHandleProps}
                >
                  {item}
                </div>
              ))}
              {provided.placeholder}
            </div>
          );
        }}
      </Draggable>
    );
  }
}

Hope this helps :)

  • 👍7
  • 👎1
  • ❤️2
alexreardon commented

Thanks @kasperpihl.

I have created a reusable example for people to play with which also works for keyboard dragging:

https://www.webpackbin.com/bins/-L-3aZ_bTMiGPl8bqlRB

Keep in mind that this is not a supported pattern and we are not testing against it. Early next year this will be done by default and as a consumer you will not need to worry about it

  • 👍1
alexreardon commented

Note to future self:

  • need to ensure focus is maintained when moving to portal as well as moving from portal if the element had focus prior to dragging
  • React.Fragment
rauleite commented

The createPortal, is client feature. I think this solution unfortunately isn't compatible with SSR.

alexreardon commented

The portal is only used while dragging. So SSR will be unaffected

rauleite commented

Uhm... with some typeof window !== 'undefined' ? document... works

I don't no if this information will help you in any way, but the new Portal implementation for Material-ui V1, is very fine. Maybe you wanna take a look.

If the container refs isn't passed, they assume the body as default.

elv1n commented

Creating element in portal will drop element in component and another element jumping
I use openPortal function like this for resolve the issue

optionalPortal(styles, element) {
    if(styles.position === 'fixed') {
     return (
			<div style={{ width: style.width, height: style.height }}>
				{createPortal(element, _dragEl)}
			</div>
		);
    }
    return element;
  }
alexreardon commented

This one is troubling me. There are a few draw backs to moving to portals:

  • Moving a single Draggable to a portal in a small list adds about 10-40%` (For a list of two cards it goes from 7ms to 12ms)
  • Moving a single Draggable to a portal in a big list adds about 70%! (For a list of 500 items it goes from 23ms to 79ms)
  • When dragging a list with lots of children (such as a column) then for a big list (125 cards) it goes from a 22ms lift to a 358ms lift (~93% increase). This is because the entire tree needs to be mounted again in the portal (previously I was mistaken in finding this to be 700ms. That was due to some extra logging)

This is understandable that the portal adds this time given that the old component tree is being unmounted and remounted into a new location. However, for our purposes this is a significant performance regression.

The ideal would be for the portal not to reprocess the tree and simply shift it to the new node..

I am not sure how to proceed. Portals are really important for style purposes, but this perf hit is hard to swallow. I was thinking of turning on portals by default and letting users opt out for high performance needs. However, given that there are fairly significant performance regressions I am not sure whether we should move to portals at this stage..

kasperpihl commented

So I’m using the framework with portals at the moment and it was not a big deal to set up, once figured out.

I vote you don’t use portals in the framework, but show a simple example how you can set that up yourself and make people aware about the performance loss.

I really love your focus on performance.

onemenny commented

@alexreardon is vanilla JS an options?
drop the Portal and do it with vanilla JS, we had a similar situation with a templated tooltip and workaround it with vanilla JS

kasperpihl commented

@onemenny might be possible I think. react-helmet is doing that.
Under the hood it uses https://github.com/gaearon/react-side-effect

alexreardon commented

For now we will not add portal support into react-beautiful-dnd itself. So we will not be adding a prop to turn on/off the usage of React.Portal. We want to have a great performance story before we allow consumers to use them. We will create an example which uses React.Portal that you manage yourself as a consumer. It will be possible to use React.Portal with react-beautiful-dnd. We will make some big warnings about React.Portal performance in the documentation.

We are still thinking about whether we should guarantee React.Portal compatibility as part of public api.

alexreardon commented

We now support consumers using their own portals see using a portal guide

zachsa commented

We now support consumers using their own portals see using a portal guide

This link is broken

  • 👍2
zachsa commented

But the example by kasperpihl seems to work fine, and is easy to setup