Improve merging of tags by simple drag and drop #144 #154
Conversation
Added drag&drop functionality Allowing sorting the tags by name, as this is more intuitive
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi, thanks a lot for working on this! I tried it locally and it's really really cool! I'll review the code now :) |
| x: 0, | ||
| y: 0, | ||
| }; | ||
| let currentState: DragState = initialState; |
There was a problem hiding this comment.
I'm not a react expert but I think global mutable state are frowned upon? Is it not possible to do this with a React.useState instead?
There was a problem hiding this comment.
I am not an expert either^^
I have however extracted it out now, since that functionality is pretty much the same for the drag&drop functionality for assigning bookmarks to lists (next PR ;-) ), so it makes sense to reuse it.
It is now more encapsulated, so I guess that concern should be gone, but let me know.
There was a problem hiding this comment.
So, after learning a lot, i found this:
useState needs to be used for the change detection to work, but the drag&drop handlers are quite slow to execute and react-draggable is not implemented that well. Due to the processing overhead, the
My followup PR for also implementing drag&drop for assigning bookmarks to lists has therefore some changes:
- The components themselves keep their state instead of the parent --> more fine grained rerender decisions possible
- most of the state is kept outside of useState and instead in a simple closure, to make sure everything is still fast enough
- the isDragging state is kept with useState, which makes a rerender possible, when the user starts to drag.
| <label> | ||
| <input | ||
| type="checkbox" | ||
| checked={draggingEnabled} | ||
| onChange={handleDraggableChange} | ||
| /> | ||
| Allow Merging via Drag&Drop | ||
| </label> | ||
| <br /> | ||
| <label> | ||
| <input | ||
| type="checkbox" | ||
| checked={sortByName} | ||
| onChange={handleSortByNameChange} | ||
| /> | ||
| Sort by Name | ||
| </label> |
There was a problem hiding this comment.
I think those can probably be shadcn toggles (https://ui.shadcn.com/docs/components/toggle) and can probably be aligned right in the page.
If UI design is not your expertise, I'm happy to implement it myself after merging your PR.
There was a problem hiding this comment.
looks like this now
| <div | ||
| className="group relative flex cursor-grab" | ||
| data-id={t.id} | ||
| onDragOver={handleDrag} | ||
| > |
There was a problem hiding this comment.
How about we pass onDragOver to TagPill and remove that div?
There was a problem hiding this comment.
turns out the onDragOver is not actually needed, so I removed it.
If I remove the div though, it is suddenly not possible to drag it anymore. I am not sure why though...
minor renamings removed some unnecessary code
extracted out the drag and drop functionality to be more encapsulated and reusable
improved the usage sorter to additionally compare by name if the usage is the same
replaced checkboxes with toggles floating on the right
|
Someone is attempting to deploy a commit to the Mohamed Bassem's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thanks a lot of addressing the comments! I think there are some minor changes (e.g. adding some icons and margins to the buttons) but I'll take care of those!
Thanks again for working on this and also giving me a tool to be able to add support for drag and drops in other places. Very cool!
| export interface DragAndDropFunctions { | ||
| handleDragStart: (e: DraggableEvent, data: DraggableData) => void; | ||
| handleDrag: (e: DraggableEvent) => void; | ||
| handleDragEnd: () => void; | ||
| dragState: DragState; | ||
| } |
There was a problem hiding this comment.
This interface seems to only be used as the return type of the useDragAndDrop hook. I think you can drop it completely and typescript will be able to infer it.
| const { dragState, handleDrag, handleDragStart, handleDragEnd } = | ||
| useDragAndDrop( | ||
| "data-id", | ||
| "data-id", | ||
| (dragSourceId: string, dragTargetId: string) => { | ||
| mergeTag({ | ||
| fromTagIds: [dragSourceId], | ||
| intoTagId: dragTargetId, | ||
| }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
This is very very cool!
|
@kamtschatka You can see some of the simplifications I made here (e5fd9ee). Mainly:
Again thanks a lot for the idea and implementation! |
|
This is what it looks like now: 
|
Added drag&drop functionality
Allowing sorting the tags by name, as this is more intuitive
Just a crude change to get some feedback. As I am no UI dev, I am thankful for any improvement ideas.