-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
RFD: Moving to a hook api 🎣 #871
Comments
Oh wow this is an interesting question. I think a hook based api would have to come along with a render prop based api. Otherwise, you wouldn't be able to consume the hooks in non-function components. I'm not 100% sure what a I would assume that it would use |
Maybe something like this? function Card({ itemId, index }) {
const draggable = useDraggable({ draggableId: itemId, index });
return (
<div {...draggable.draggableProps} ref={draggable.innerRef}>
<h1 {...draggable.dragHandleProps}>Drag me by the title</h1>
<p>...</p>
</div>
);
} I mean, this is just syntactically how I imagine it. Not sure how to make it workable, not familiar with the inner workings of this lib (would love to but haven't had time). |
Yup, and if we were sticking with the way that
that way you could easily rename, and it is similar to the way react core works |
I wouldn't extrapolate that feature of With something like this hypothetical |
@gnapse definitely, however the way I picture it, useDraggable would only return two things. For example, check this from the docs. import { Draggable } from 'react-beautiful-dnd';
<Draggable draggableId="draggable-1" index={0}>
{(provided, snapshot) => (
<div
ref={provided.innerRef}
{...provided.draggableProps}
{...provided.dragHandleProps}
>
<h4>My draggable</h4>
</div>
)}
</Draggable>; Here we get a Unless you are suggesting all the properties get merged on one |
Oh well, yes. Maybe. Makes more sense now. |
This seems like a reasonable API function Card({ itemId, index }) {
const [provided, snapshot] = useDraggable({ draggableId: itemId, index });
return (
<div {...provided.draggableProps} {...provided.dragHandleProps } ref={provided.innerRef}>
Drag me!
</div>
);
} and function List({ listId }) {
const [provided, snapshot] = useDroppable({ draggableId: itemId, index });
return (
<div {...provided.droppableProps} ref={provided.innerRef}>
<p>...</p>
{provided.placeholder}
</div>
);
} It seems to be the hook equivalent of the render props api we are providing now |
I like this more: const {provided, snapshot} = useDraggable({ draggableId: itemId, index }); It is not even a single character longer and serves probably >90% of use cases. I'd argue that the minor advantage of easily renaming the objects returned is not worth the disadvantages returning an array:
The likelihood of even wanting to rename it is small so I think staying with object spreading and keeping the current names as a convention is better. If you really feel that you need to rename it you could still do: const {provided: renamedProvided, snapshot: renamedSnapshot} = useDraggable({ draggableId: itemId, index }); Not too hard if you ask me.
In short that means that in the majority of the cases you actually want to rename the values returned by I don't see that being true for higher level hooks like |
I feel the same way. I fear that because of You hardly ever use more than one draggable in the same context, and the same goes for droppable. And even if you did, you'd be better off not destructuring it, give the return value two meaningful names, and use those names' properties in the context they need to be used. |
While this does look awesome from the outside, I'm curious how we are going to achieve this effect. Draggable does a lot more then just feed in those items to it's children, it also wraps those children in the handle as well as the dimension publisher. I would argue that this is a case where render props/children as a function makes more sense then using hooks, since there is some hierarchy and extra rendering happening. Any ideas how we could even begin to achieve this effect with hooks? |
I actually do agree with the object notation over the array notation in this usecase, gives more extendability down the line. One thing that is a bit more common then double draggable or double droppable is a draggable inside of a droppable in the same render. But the Droppable and Draggable components expose a provided and a snapshot property. However, these cases don't feel common enough to warrant array notation |
That is an interesting question. I guess it should be possible to rewrite the As far as I understood you should be able to transform any class based component logic into a hook including state and lifecycle methods and effectively make the components obsolete. The only problem would be if a component renders a context provider. You can't replace that with a hook. For const useDraggableDimensionPublisher = ({draggableId, droppableId, type, index, ref}) => {
// implementation of dimension publisher
// probably uses `useEffect()` to implement lifecycle side effects
useEffect(/* lifecycle implementation here */)
}
const useDraggable = ({draggableId, index}) => {
const {droppableId, type} = useContext(/* ... */);
const draggableRef = useRef();
// calls dimension publisher hook to use side effects
useDraggableDimensionPublisher({draggableId, droppableId, type, index, ref: draggableRef.current})
// implementation of draggable ...
return {provided, snapshot};
} |
My current thinking:
// Assuming a useDraggable(props) hook extends
function Draggable({draggableId, type, children}) {
const [provided, snapshot] = useDraggable({draggableId, type});
return children(provided, snapshot);
} |
I'd love to get working on some of these hooks, what hooks would make the most sense to start with? |
Some bad news: there is no useProvider react hook. So there is no way for a hook to setup context. This is bad because While we can still move internally to hooks, this might be a dealbreaker to exposing a hook based public api |
Could that context information instead be shifted up to DragDropContext and contained in some sort of internal map by droppable id? |
Interesting @YurkaninRyan |
It then turns into a hot mess when you start to drag |
I have a branch that is working and passing all tests. I am doing some finial performance testing #1208. You can play with it here: https://deploy-preview-1208--react-beautiful-dnd.netlify.com/?path=/story/single-vertical-list--basic The branch is currently using |
|
You can play with the new hooks based implementation: yarn add react-beautiful-dnd@11.0.0-beta 😍 |
Until we a hook can setup context, we will not be moving to a hook based public API. We might investigate workarounds, but for now the render prop API will remain |
Pardon me for intruding, but I don't understand the need for the missing
Droppable is already rendering a Am I missing something here? |
We cannot have a |
Ah, I didn't realize you were trying to make Thanks for the clarification! tl;dr Original syntax import { useState } from 'react'
import { DragDropContext, Draggable, Droppable } from 'react-beautiful-dnd'
export default function App () {
const [items, setItems] = useState(['one', 'two', 'three'])
function onDragEnd ({ type, reason, destination, source }) {
if (type === 'DEFAULT' && reason === 'DROP' && destination) {
const newItems = Array.from(items)
const moved = newItems.splice(source.index, 1)
newItems.splice(destination.index, 0, moved)
setItems(newItems)
}
}
return (
<DragDropContext onDragEnd={onDragEnd}>
<Droppable droppableId='dropzone'>
{(droppableProvided, droppableSnapshot) => {
return (
<ul ref={droppableProvided.innerRef}>
{items.map((item, idx) => {
return (
<Draggable draggableId={`draggable-${item}`} index={idx}>
{(draggableProvided, draggableSnapshot) => {
return (
<li
key={item}
ref={draggableProvided.innerRef}
{...draggableProvided.draggableProps}
{...draggableProvided.dragHandleProps}
>{item}</li>
)
}}
</Draggable>
)
})}
{droppableProvided.placeholder}
</ul>
)
}}
</Droppable>
</DragDropContext>
)
} Potential new syntax import { useState } from 'react'
import { useDragDrop } from 'react-beautiful-dnd'
export default function App () {
const [items, setItems] = useState(['one', 'two', 'three'])
const [DragDropContext, makeDroppable, makeDraggable] = useDragDrop()
function onDragEnd ({ type, reason, destination, source }) {
if (type === 'DEFAULT' && reason === 'DROP' && destination) {
const newItems = Array.from(items)
const moved = newItems.splice(source.index, 1)
newItems.splice(destination.index, 0, moved)
setItems(newItems)
}
}
const [Droppable, droppableProvided, droppableSnapshot] = makeDroppable({
id: 'dropzone'
})
return (
<DragDropContext onDragEnd={onDragEnd}>
<Droppable>
<ul ref={droppableProvided.innerRef}>
{items.map((item, idx) => {
const [Draggable, draggableProvided, draggableSnapshot] = makeDraggable({
id: `draggable-${item}`,
index: idx
})
return (
<Draggable>
<li
key={item}
ref={draggableProvided.innerRef}
{...draggableProvided.draggableProps}
{...draggableProvided.dragHandleProps}
>{item}</li>
</Draggable>
)
})}
{droppableProvided.placeholder}
</ul>
</Droppable>
</DragDropContext>
)
} Again, I'm not sure how good of an idea this is, I'm just trying to help generate ideas... |
An interesting idea @shawninder. I did consider it, but I was not sure there was much value in this pattern over just keeping with the existing render props api. The internal hook refactor has shipped in 11.0 For now, until a hook can setup context we won't be able to move to a public hook api. Based on that I will close this issue. I gave it a really good shot! |
It seems that |
It might make sense for hooks not to be able to set up context as an architectural decision. The consequence of the decision is that we cannot move to a purely hooks based api. It is not the end of the world 😊 |
Why does the API need to be a purely hooks API though? Why not expose just I do agree it’s not a big deal though, even with hooks being a thing, the render prop API still makes sense when the abstraction is a visual one and not a logical one, which is the case for drag and drop. Has it been explored to have a potential |
Ok, this will be my last attempt with
function useProvider (Context, value) {
return (children) => {
return (
<Context.Provider value={value}>{children}</Context.Provider>
)
}
}
function useDroppable ({ id }) {
const droppable = useProvider(DroppableContext, { ... })
...
return [droppable, droppableProvided, droppableSnapshot]
}
funciton useDraggable ({ id }) {
const { fromContext } = useContext(DroppableContext)
...
return [draggable, draggableProvided, draggableSnapshot]
} Usage: import { useState } from 'react'
import { DragDropContext, useDroppable, useDraggable } from 'react-beautiful-dnd'
export default function App () {
const [items, setItems] = useState(['one', 'two', 'three'])
function onDragEnd ({ type, reason, destination, source }) {
if (type === 'DEFAULT' && reason === 'DROP' && destination) {
const newItems = Array.from(items)
const moved = newItems.splice(source.index, 1)
newItems.splice(destination.index, 0, moved)
setItems(newItems)
}
}
const [droppable, droppableProvided, droppableSnapshot] = useDroppable({
id: 'dropzone'
})
return (
<DragDropContext onDragEnd={onDragEnd}>
{droppable(
<ul ref={droppableProvided.innerRef}>
{items.map((item, idx) => {
const [draggable, draggableProvided, draggableSnapshot] = useDraggable({
id: `draggable-${item}`,
index: idx
})
return draggable(
<li
key={item}
ref={draggableProvided.innerRef}
{...draggableProvided.draggableProps}
{...draggableProvided.dragHandleProps}
>{item}</li>
)
})}
{droppableProvided.placeholder}
</ul>
)}
</DragDropContext>
)
} Working example of I don't think this changes the conclusion for this repo, but it may be interesting for people following this discussion. |
So what is actual status of this? I can see Hooks PR merged, but I don't really see it in Docs etc. |
As far as I know there is no user facing hooks API yet because react-beautiful-dnd uses react context and there is no way for a hook to set up a context. For that you need to render a The PR only replaced some internal logic that has been managed in class based components with hooks. |
React hooks
have recently been announced.Would we benefit from moving from a render props api to a hook based api for
react-beautiful-dnd
? If so, what would it even look like?I have not thought very much about this, but I wanted to open it for discussion
The text was updated successfully, but these errors were encountered: