-
Notifications
You must be signed in to change notification settings - Fork 16.3k
refactor(draggable filters): react-dnd to dnd-kit migration for DraggableFilters #36301
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
base: msyavuz/chore/react-18
Are you sure you want to change the base?
refactor(draggable filters): react-dnd to dnd-kit migration for DraggableFilters #36301
Conversation
|
Bito Automatic Review Skipped - Branch Excluded |
|
Looks like pre-commit needs to be run, and that will likely clear some of the other CI issues. Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the DraggableFilters component from the unmaintained react-dnd library to the modern dnd-kit library. The migration updates the drag-and-drop implementation to use hooks-based APIs while maintaining the existing behavior of making the entire container draggable.
Key changes:
- Replaced react-dnd hooks (
useDrag,useDrop) with dnd-kit'suseSortablehook - Implemented pointer sensor with 5px activation constraint to prevent conflicts with delete/undo button click handlers
- Updated parent component to use
DndContextandSortableContextwrappers
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| FilterTitleContainer.tsx | Wraps filter list with DndContext and SortableContext, adds handleDragEnd handler and sensor configuration |
| DraggableFilter.tsx | Replaces react-dnd implementation with dnd-kit's useSortable hook, simplifies drag logic |
Comments suppressed due to low confidence (1)
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx:166
- The
indexandonRearrangeprops are still being passed toDraggableFilterbut are no longer used by the component after the dnd-kit migration. Remove these props from the parent component call once the corresponding interface in DraggableFilter.tsx is updated.
<DraggableFilter
key={item}
onRearrange={onRearrange}
index={index}
filterIds={[item]}
>
| import React, { useRef, FC } from 'react'; | ||
| import { styled } from '@apache-superset/core/ui'; | ||
| import { | ||
| DragSourceMonitor, | ||
| DropTargetMonitor, | ||
| useDrag, | ||
| useDrop, | ||
| XYCoord, | ||
| } from 'react-dnd'; | ||
| import { FC } from 'react'; |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate import of FC from React. Line 19 imports both useRef and FC, while line 21 imports FC again. Remove the duplicate import on line 21 and the unused useRef import from line 19.
| index: number; | ||
| filterIds: string[]; | ||
| onRearrange: (dragItemIndex: number, targetIndex: number) => void; |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The index and onRearrange props are defined in the interface but are no longer used in the component after migrating to dnd-kit. These should be removed from the interface since the parent component now handles rearrangement through the handleDragEnd function.
| index: number; | |
| filterIds: string[]; | |
| onRearrange: (dragItemIndex: number, targetIndex: number) => void; | |
| filterIds: string[]; |
| <div css={{ flex: 1 }}>{children}</div> | ||
| </Container> | ||
| ); | ||
| ) |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the return statement. Add a semicolon after the closing parenthesis for consistency with the rest of the codebase.
| ) | |
| ); |
|
Aside from the pre-commit needs, it looks like some of Copilot's notes are pretty sensible. Let us know if you think otherwise. |
SUMMARY
Since react-dnd is no longer maintained, dnd-kit is being used as a replacement, as it is modern with hooks and is maintained regularly. With dnd-kit, I had two options: either make the dragIcon draggable or make the container draggable. I went with making the container draggable, as this was the previous behaviour.
Making the container draggable came with some challenges, as the container contains a delete button and an undo button (after deletion). These buttons have their listeners, like onClick, which caused an issue as we wrap the whole container with dnd-kit listeners. So, if we click on the delete button, it won't trigger, as all the events are captured by the dnd-kit listener and are never passed down to the delete and undo buttons. So for this, we used sensors to make the draggable action active based on the distance of drag, i.e., move the container by 5 px, to make the draggable action active, which would make the dnd listener catch that event otherwise, the events are passed to delete and undo and work as expected.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE


AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION