Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to portal solution for Draggables #192

Closed
onemenny opened this issue Nov 22, 2017 · 20 comments
Closed

Move to portal solution for Draggables #192

onemenny opened this issue Nov 22, 2017 · 20 comments

Comments

@onemenny
Copy link

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
Copy link
Collaborator

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.

@alexreardon alexreardon changed the title Warning: position fixed and transitions Move to portal solution for Draggables Nov 22, 2017
@onemenny
Copy link
Author

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

@kasperpihl
Copy link

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 :)

@alexreardon
Copy link
Collaborator

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

@alexreardon
Copy link
Collaborator

I have added a note in the docs about this: https://github.com/atlassian/react-beautiful-dnd#warning-position-fixed

@alexreardon
Copy link
Collaborator

alexreardon commented Nov 29, 2017

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
Copy link

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

@alexreardon
Copy link
Collaborator

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

@rauleite
Copy link

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
Copy link

elv1n commented Feb 7, 2018

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;
  }

@evelant
Copy link

evelant commented Feb 22, 2018

@alexreardon Your example link is broken. https://www.webpackbin.com/bins/-L-3aZ_bTMiGPl8bqlRB

@alexreardon alexreardon mentioned this issue Mar 25, 2018
21 tasks
@alexreardon
Copy link
Collaborator

alexreardon commented Mar 28, 2018

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
Copy link

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
Copy link
Author

@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
Copy link

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

@alexreardon
Copy link
Collaborator

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.

@kuzmenko1256
Copy link

@alexreardon
Copy link
Collaborator

alexreardon commented Apr 14, 2018

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

@zachsa
Copy link

zachsa commented Jan 30, 2020

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

This link is broken

@zachsa
Copy link

zachsa commented Jan 30, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants