Remove placeholder from API once upgraded to React 16

[alexreardon]

Droppable API

<Droppable droppableId="droppable-1">
  {(provided, snapshot) => (
    <div ref={provided.innerRef}>
      Good to go
    </div>
  )}
</Droppable>

Proposed change: rather that requiring the consumer to bind the ref - use ReactDOM.find(this) to get the DOM node.

New usage

<Droppable droppableId="droppable-1">
  {(provided, snapshot) => (
    <div>
      Good to go
    </div>
  )}
</Droppable>

Draggable API

<Draggable draggableId="draggable-1">
  {(provided, snapshot) => (
    <div>
      <div
        ref={provided.innerRef}
        style={provided.draggableStyle}
        {...provided.dragHandleProps}
      >
        Drag me!
      </div>
      {provided.placeholder}
    </div>
  )}
</Draggable>
```js

Again, rather than the user needing to manually return the ref - just use ReactDOM to get it.
Also - placeholder will be come a dynamically added sibling to the root node rather than needing to be put into the react tree by the consumer. However, this needs to wait until we upgrade to React 16 so that we can return siblings without a wrapping element.

**new usage:**

```js
<Draggable draggableId="draggable-1">
  {(provided, snapshot) => (
      <div
        style={provided.draggableStyle}
        {...provided.dragHandleProps}
      >
        Drag me!
      </div>
  )}
</Draggable>

[tech4him1]

Now that React 16 is released, is someone working on this, or do you need someone to take it on?

[alexreardon]

The original plan is to ensure that the current api works with React 15 and React 16. I have already checked so all should be good 🤞 . After that we can make the jump to leaning on the React 16 children behaviour. This will need to be a planned move though as it will break React 15 users. We may try to find a way to still be compatible with React 15 - but we'll see how we go.

[bradleyayers]

I spiked implementing this as a layer on top of react-beautiful-dnd, and all seemed well at first, but I eventually ran into some wrinkles when trying to use React.Component children instead of React.DOMElement children.

The current API is nice because you have control over how refs are wired, and can choose to support innerRef in your own components and use them as an immediate children. But when trying to automatically wire refs using React.cloneElement(props.children(provided, snapshot), { ref: innerRef }), you need to mandate that the immediate child is a React.DOMElement so that you're guaranteed that the ref prop will be given an HTMLElement.

I think this would be a problem when using something like styled-components, as you would need to wire up innerRef rather than ref. Perhaps it's possible to support the current API and the new API, but I suspect it might end up being more complicated to consumers.

I'd be interested in hearing how other people go with this, maybe I just made a mistake somewhere…

[damarna85]

hey @alexreardon, do you have any expected date to have the react 16 compatible version? Thanks a lot and congrats for such a great work :)

[alexreardon]

Thank you. It should be compatible now. This issue regards cleaning up a part of the api to make it more friendly for react 16. However, doing this will mean that it will not be compatible will react 15 any more. For now we won’t change it, but will probably will in the coming months

[Goues]

Hello, will this change allow me to use a dropabble that is also a draggable with no extra markup? My idea is deeply nested elements (root -> parents -> children -> grand-children) where everything is sortable (I can sort grand-children in children, but I can also sort children in parents and parents in root). As far as I understand, I cannot do something like:

<DragDropContext>
	<div className="root">
		<Droppable>
			<Draggable>
				<div className="parent">
					<Droppable>
						<Draggable>
							<div className="child" />
						</Draggable>
						<Draggable>
							<div className="child" />
						</Draggable>
						<Draggable>
							<div className="child" />
						</Draggable>
					</Droppable>
				</div>
			</Draggable>
			...more parent like this
		</Droppable>
	</Root>
</DragDropContext>

because my parent would have to be reffed twice, both by draggable's and droppable's innerRef.

Is this going to be possible? I can't use extra divs because of styles...

[souporserious]

I'd love to see provided.innerRef stick around. There is a chance findDOMNode could be deprecated and it doesn't seem to play nice with some libraries like Jest.

[alexreardon]

We will not be moving to findDOMNode and will be keeping provided.innerRef #403 👍

[alexreardon]

But we are still planning on removing the placeholder from the Draggable api

[alexreardon]

New Draggable api:

<Draggable draggableId="draggable-1" index={0}>
  {(provided, snapshot) => (
-  <div>
      <div
        ref={provided.innerRef}
        {...provided.draggableProps}
        {...provided.dragHandleProps}
      >
        Drag me!
      </div>
-    {provided.placeholder}
-   </div>
  )}
</Draggable>;

While we are not switching to Portals straight away, this new api is totally compatible with moving to React.Portal

[alexreardon]

The draw back to this change is that there is less control over where the placeholder is placed in the React tree. It will now always be a sibling of the React.Node returned by the Draggable.

The alternative is to provide the placeholder and put the mandate on the consumer to manage React.Portal and React.Fragment. However, I think that it would be too much to do to get a Draggable going. The new api will allow us to move to use React.Portal without any changes to the api

I think that this api change should not be a problem for consumers - but it might be a minor cause of friction when upgrading.

[alexreardon]

/cc @bradleyayers @souporserious ^

[GandalfM]

This can be a problem for our usage in draggable table.

Pseudo code:

<Draggable>
    {(provided, snapshot) => [
        <tr ref={provided.innerRef}>
            // content
        </tr>,

        provided.placeholder ? (
            <tr>
                <td>
                    {provided.placeholder}
                </td>
            </tr>
        ) : null,
    ]}
</Draggable>;

We wrapped placeholder in <tr> and <td> tags. When placeholder was the direct sibling of the React.Node returned by the Draggable height of placeholder was not computed properly. Additionally, the structure of HTML tags was wrong (div created by placeholder was a direct child of tbody).

Do you think that it is possible to switch to new API by default, but also leave the possibility to specify position of placeholder if needed? :)

[alexreardon]

We are giving a lot of love to tables in this upgrade ❤️

Firstly, you no longer need to worry about a placeholder being a div. It will now have the same tagName as the element you are dragging. If you are dragging a tr, the placeholder will be a tr. Secondly, you no longer need to worry about needing to create wrapping elements around a Draggable.

Tables are also getting their own guide. The guide will explain how you can maintain the dimensions of everything while you are dragging.

Here is one of the table examples we have put together: table example. It is not the most performant on lift but it is fine for <80 items. For high performance needs we are putting together some other examples.

[alexreardon]

Closed by #406

[bradleyayers]

Retracting control over placeholder rendering should be fine for how we're using it in Dovetail. We're not doing anything funky so I'm expecting it will "just work".

[corbanbrook]

Are custom Draggable placeholders no longer going to be supported?

[towfiqi]

Custom Draggable placeholders are no longer supported... is there any alternative?