This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch refactor/explore-controls-dnd-kit in repository https://gitbox.apache.org/repos/asf/superset.git
commit bf1b10a2d03a6a4bf06ede13eff8bc429d11343a Author: Evan Rusackas <[email protected]> AuthorDate: Tue Feb 10 23:43:21 2026 -0800 refactor(explore): migrate Explore Controls from react-dnd to @dnd-kit Migrate the Explore Controls drag-and-drop functionality from react-dnd to @dnd-kit for React 18 compatibility. This includes: - OptionWrapper: sortable column/metric options - OptionControlLabel: sortable metric labels - DndSelectLabel: SortableContext wrapper for animations - DatasourcePanelDragOption: datasource panel drag source - ExploreDndContext: new DndContext wrapper with drag handlers Also updates test helpers to support @dnd-kit via `useDndKit: true` option and updates all related test files. Co-Authored-By: Claude Opus 4.5 <[email protected]> --- superset-frontend/spec/helpers/testing-library.tsx | 7 + .../DatasourcePanelDragOption/index.tsx | 41 +++- .../ExploreContainer/ExploreContainer.test.tsx | 54 +---- .../ExploreContainer/ExploreDndContext.tsx | 255 +++++++++++++++++++++ .../explore/components/ExploreContainer/index.tsx | 80 ++----- .../components/controls/ContourControl/index.tsx | 2 + .../DndColumnSelectControl/DndColumnSelect.tsx | 5 + .../DndColumnSelectControl/DndFilterSelect.tsx | 2 + .../DndColumnSelectControl/DndMetricSelect.tsx | 5 + .../DndColumnSelectControl/DndSelectLabel.test.tsx | 9 +- .../DndColumnSelectControl/DndSelectLabel.tsx | 84 +++++-- .../DndColumnSelectControl/OptionWrapper.test.tsx | 55 ++++- .../DndColumnSelectControl/OptionWrapper.tsx | 90 +++----- .../OptionControls/OptionControls.test.tsx | 21 +- .../components/controls/OptionControls/index.tsx | 109 ++++----- 15 files changed, 515 insertions(+), 304 deletions(-) diff --git a/superset-frontend/spec/helpers/testing-library.tsx b/superset-frontend/spec/helpers/testing-library.tsx index 21abd6a44d0..a83590edbdd 100644 --- a/superset-frontend/spec/helpers/testing-library.tsx +++ b/superset-frontend/spec/helpers/testing-library.tsx @@ -37,6 +37,7 @@ import { BrowserRouter } from 'react-router-dom'; import { Provider } from 'react-redux'; import { DndProvider } from 'react-dnd'; import { HTML5Backend } from 'react-dnd-html5-backend'; +import { DndContext } from '@dnd-kit/core'; import reducerIndex from 'spec/helpers/reducerIndex'; import { QueryParamProvider } from 'use-query-params'; import { ReactRouter5Adapter } from 'use-query-params/adapters/react-router-5'; @@ -48,6 +49,7 @@ import { ExtensionsProvider } from 'src/extensions/ExtensionsContext'; type Options = Omit<RenderOptions, 'queries'> & { useRedux?: boolean; useDnd?: boolean; + useDndKit?: boolean; // Use @dnd-kit instead of react-dnd useQueryParams?: boolean; useRouter?: boolean; useTheme?: boolean; @@ -75,6 +77,7 @@ export const defaultStore = createStore(); export function createWrapper(options?: Options) { const { useDnd, + useDndKit, useRedux, useQueryParams, useRouter, @@ -99,6 +102,10 @@ export function createWrapper(options?: Options) { ); } + if (useDndKit) { + result = <DndContext>{result}</DndContext>; + } + if (useDnd) { result = <DndProvider backend={HTML5Backend}>{result}</DndProvider>; } diff --git a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx index 7540b235513..22a1c2dc384 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragOption/index.tsx @@ -16,8 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { RefObject } from 'react'; -import { useDrag } from 'react-dnd'; +import { RefObject, useMemo } from 'react'; +import { useDraggable } from '@dnd-kit/core'; import { Metric } from '@superset-ui/core'; import { css, styled, useTheme } from '@apache-superset/core/ui'; import { ColumnMeta } from '@superset-ui/chart-controls'; @@ -30,8 +30,8 @@ import { Icons } from '@superset-ui/core/components/Icons'; import { DatasourcePanelDndItem } from '../types'; -const DatasourceItemContainer = styled.div` - ${({ theme }) => css` +const DatasourceItemContainer = styled.div<{ isDragging?: boolean }>` + ${({ theme, isDragging }) => css` display: flex; align-items: center; justify-content: space-between; @@ -44,6 +44,8 @@ const DatasourceItemContainer = styled.div` color: ${theme.colorText}; background-color: ${theme.colorBgLayout}; border-radius: 4px; + cursor: ${isDragging ? 'grabbing' : 'grab'}; + opacity: ${isDragging ? 0.5 : 1}; &:hover { background-color: ${theme.colorPrimaryBgHover}; @@ -70,14 +72,23 @@ export default function DatasourcePanelDragOption( ) { const { labelRef, showTooltip, type, value } = props; const theme = useTheme(); - const [{ isDragging }, drag] = useDrag({ - item: { - value: props.value, - type: props.type, + + // Create a unique ID for this draggable item + const draggableId = useMemo(() => { + if (type === DndItemType.Column) { + const col = value as ColumnMeta; + return `datasource-${type}-${col.column_name || col.verbose_name}`; + } + const metric = value as MetricOption; + return `datasource-${type}-${metric.metric_name || metric.label}`; + }, [type, value]); + + const { attributes, listeners, setNodeRef, isDragging } = useDraggable({ + id: draggableId, + data: { + type, + value, }, - collect: monitor => ({ - isDragging: monitor.isDragging(), - }), }); const optionProps = { @@ -87,7 +98,13 @@ export default function DatasourcePanelDragOption( }; return ( - <DatasourceItemContainer data-test="DatasourcePanelDragOption" ref={drag}> + <DatasourceItemContainer + data-test="DatasourcePanelDragOption" + ref={setNodeRef} + isDragging={isDragging} + {...attributes} + {...listeners} + > {type === DndItemType.Column ? ( <StyledColumnOption column={value as ColumnMeta} {...optionProps} /> ) : ( diff --git a/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx b/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx index c8bf92caa41..c42d18a1d7c 100644 --- a/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreContainer/ExploreContainer.test.tsx @@ -18,12 +18,8 @@ */ import { useContext } from 'react'; import { fireEvent, render } from 'spec/helpers/testing-library'; -import { OptionControlLabel } from 'src/explore/components/controls/OptionControls'; import ExploreContainer, { DraggingContext, DropzoneContext } from '.'; -import OptionWrapper from '../controls/DndColumnSelectControl/OptionWrapper'; -import DatasourcePanelDragOption from '../DatasourcePanel/DatasourcePanelDragOption'; -import { DndItemType } from '../DndItemType'; const MockChildren = () => { const dragging = useContext(DraggingContext); @@ -57,58 +53,21 @@ test('should render children', () => { <ExploreContainer> <MockChildren /> </ExploreContainer>, - { useRedux: true, useDnd: true }, + { useRedux: true }, ); expect(getByTestId('mock-children')).toBeInTheDocument(); expect(getByText('not dragging')).toBeInTheDocument(); }); -test('should only propagate dragging state when dragging the panel option', () => { - const defaultProps = { - label: <span>Test label</span>, - tooltipTitle: 'This is a tooltip title', - onRemove: jest.fn(), - onMoveLabel: jest.fn(), - onDropLabel: jest.fn(), - type: 'test', - index: 0, - }; +test('should initially have dragging set to false', () => { const { container, getByText } = render( <ExploreContainer> - <DatasourcePanelDragOption - value={{ metric_name: 'panel option', uuid: '1' }} - type={DndItemType.Metric} - /> - <OptionControlLabel - {...defaultProps} - index={1} - label={<span>Metric item</span>} - /> - <OptionWrapper - {...defaultProps} - index={2} - label="Column item" - clickClose={() => {}} - onShiftOptions={() => {}} - /> <MockChildren /> </ExploreContainer>, - { - useRedux: true, - useDnd: true, - }, + { useRedux: true }, ); expect(container.getElementsByClassName('dragging')).toHaveLength(0); - fireEvent.dragStart(getByText('panel option')); - expect(container.getElementsByClassName('dragging')).toHaveLength(1); - fireEvent.dragEnd(getByText('panel option')); - fireEvent.dragStart(getByText('Metric item')); - expect(container.getElementsByClassName('dragging')).toHaveLength(0); - fireEvent.dragEnd(getByText('Metric item')); - expect(container.getElementsByClassName('dragging')).toHaveLength(0); - // don't show dragging state for the sorting item - fireEvent.dragStart(getByText('Column item')); - expect(container.getElementsByClassName('dragging')).toHaveLength(0); + expect(getByText('not dragging')).toBeInTheDocument(); }); test('should manage the dropValidators', () => { @@ -116,10 +75,7 @@ test('should manage the dropValidators', () => { <ExploreContainer> <MockChildren2 /> </ExploreContainer>, - { - useRedux: true, - useDnd: true, - }, + { useRedux: true }, ); expect(queryByText('test_item_1')).not.toBeInTheDocument(); diff --git a/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx b/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx new file mode 100644 index 00000000000..7be0db68f5a --- /dev/null +++ b/superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx @@ -0,0 +1,255 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + createContext, + useContext, + useState, + useCallback, + useMemo, + FC, + Dispatch, + useReducer, +} from 'react'; +import { + DndContext, + useSensor, + useSensors, + PointerSensor, + DragStartEvent, + DragEndEvent, + UniqueIdentifier, +} from '@dnd-kit/core'; +import { DatasourcePanelDndItem } from '../DatasourcePanel/types'; + +/** + * Type for the active drag item data + */ +export interface ActiveDragData { + type: string; + value?: unknown; + dragIndex?: number; + // For sortable items - callback to handle reorder + onShiftOptions?: (dragIndex: number, hoverIndex: number) => void; + onMoveLabel?: (dragIndex: number, hoverIndex: number) => void; + onDropLabel?: () => void; +} + +/** + * Context to track if something is being dragged (for visual feedback) + */ +export const DraggingContext = createContext(false); + +/** + * Context to access the currently active drag item + */ +export const ActiveDragContext = createContext<ActiveDragData | null>(null); + +/** + * Dropzone validation - used by controls to register what they can accept + */ +type CanDropValidator = (item: DatasourcePanelDndItem) => boolean; +type DropzoneSet = Record<string, CanDropValidator>; +type Action = { key: string; canDrop?: CanDropValidator }; + +export const DropzoneContext = createContext<[DropzoneSet, Dispatch<Action>]>([ + {}, + () => {}, +]); + +const dropzoneReducer = (state: DropzoneSet = {}, action: Action) => { + if (action.canDrop) { + return { + ...state, + [action.key]: action.canDrop, + }; + } + if (action.key) { + const newState = { ...state }; + delete newState[action.key]; + return newState; + } + return state; +}; + +/** + * Context for handling drag end events - controls register their onDrop handlers + */ +type DropHandler = ( + activeId: UniqueIdentifier, + overId: UniqueIdentifier, + activeData: ActiveDragData, +) => void; +type DropHandlerSet = Record<string, DropHandler>; + +export const DropHandlersContext = createContext<{ + register: (id: string, handler: DropHandler) => void; + unregister: (id: string) => void; +}>({ + register: () => {}, + unregister: () => {}, +}); + +interface ExploreDndContextProps { + children: React.ReactNode; +} + +/** + * DnD context provider for the Explore view. + * Wraps @dnd-kit/core's DndContext and provides: + * - Dragging state tracking (for visual feedback) + * - Dropzone registration (for validation) + * - Drop handler registration (for handling drops) + */ +export const ExploreDndContextProvider: FC<ExploreDndContextProps> = ({ + children, +}) => { + const [isDragging, setIsDragging] = useState(false); + const [activeData, setActiveData] = useState<ActiveDragData | null>(null); + const [dropHandlers, setDropHandlers] = useState<DropHandlerSet>({}); + + const dropzoneValue = useReducer(dropzoneReducer, {}); + + // Configure sensors for drag detection + const sensors = useSensors( + useSensor(PointerSensor, { + activationConstraint: { + distance: 5, // 5px movement required before drag starts + }, + }), + ); + + const handleDragStart = useCallback((event: DragStartEvent) => { + const { active } = event; + const data = active.data.current as ActiveDragData | undefined; + + // Don't set dragging state for reordering within a list + if (data && 'dragIndex' in data) { + return; + } + + setIsDragging(true); + setActiveData(data || null); + }, []); + + const handleDragEnd = useCallback( + (event: DragEndEvent) => { + const { active, over } = event; + + setIsDragging(false); + setActiveData(null); + + if (over && active.id !== over.id) { + const activeDataCurrent = active.data.current as + | ActiveDragData + | undefined; + const overDataCurrent = over.data.current as ActiveDragData | undefined; + + // Check if this is a sortable reorder operation + // Both items need dragIndex and the same type + if ( + activeDataCurrent && + overDataCurrent && + typeof activeDataCurrent.dragIndex === 'number' && + typeof overDataCurrent.dragIndex === 'number' && + activeDataCurrent.type === overDataCurrent.type + ) { + const { dragIndex } = activeDataCurrent; + const hoverIndex = overDataCurrent.dragIndex; + + // Call the appropriate reorder callback + const reorderCallback = + activeDataCurrent.onShiftOptions || activeDataCurrent.onMoveLabel; + if (reorderCallback) { + reorderCallback(dragIndex, hoverIndex); + } + + // Call onDropLabel if provided (for finalization after reorder) + activeDataCurrent.onDropLabel?.(); + return; + } + + // Handle external drop (from DatasourcePanel to dropzone) + const overId = String(over.id); + const handler = dropHandlers[overId]; + + if (handler && activeDataCurrent) { + handler(active.id, over.id, activeDataCurrent); + } + } + }, + [dropHandlers], + ); + + const handleDragCancel = useCallback(() => { + setIsDragging(false); + setActiveData(null); + }, []); + + const registerDropHandler = useCallback( + (id: string, handler: DropHandler) => { + setDropHandlers(prev => ({ ...prev, [id]: handler })); + }, + [], + ); + + const unregisterDropHandler = useCallback((id: string) => { + setDropHandlers(prev => { + const newHandlers = { ...prev }; + delete newHandlers[id]; + return newHandlers; + }); + }, []); + + const dropHandlersContextValue = useMemo( + () => ({ + register: registerDropHandler, + unregister: unregisterDropHandler, + }), + [registerDropHandler, unregisterDropHandler], + ); + + return ( + <DndContext + sensors={sensors} + onDragStart={handleDragStart} + onDragEnd={handleDragEnd} + onDragCancel={handleDragCancel} + > + <DropzoneContext.Provider value={dropzoneValue}> + <DropHandlersContext.Provider value={dropHandlersContextValue}> + <DraggingContext.Provider value={isDragging}> + <ActiveDragContext.Provider value={activeData}> + {children} + </ActiveDragContext.Provider> + </DraggingContext.Provider> + </DropHandlersContext.Provider> + </DropzoneContext.Provider> + </DndContext> + ); +}; + +/** + * Hook to check if something is currently being dragged + */ +export const useIsDragging = () => useContext(DraggingContext); + +/** + * Hook to get the active drag data + */ +export const useActiveDrag = () => useContext(ActiveDragContext); diff --git a/superset-frontend/src/explore/components/ExploreContainer/index.tsx b/superset-frontend/src/explore/components/ExploreContainer/index.tsx index 5cf714ea740..69f37b9532e 100644 --- a/superset-frontend/src/explore/components/ExploreContainer/index.tsx +++ b/superset-frontend/src/explore/components/ExploreContainer/index.tsx @@ -16,28 +16,17 @@ * specific language governing permissions and limitations * under the License. */ -import { - createContext, - useEffect, - useState, - Dispatch, - FC, - useReducer, -} from 'react'; - +import { FC } from 'react'; import { styled } from '@apache-superset/core/ui'; -import { useDragDropManager } from 'react-dnd'; -import { DatasourcePanelDndItem } from '../DatasourcePanel/types'; +import { + ExploreDndContextProvider, + DraggingContext, + DropzoneContext, +} from './ExploreDndContext'; -type CanDropValidator = (item: DatasourcePanelDndItem) => boolean; -type DropzoneSet = Record<string, CanDropValidator>; -type Action = { key: string; canDrop?: CanDropValidator }; +// Re-export contexts for backward compatibility +export { DraggingContext, DropzoneContext }; -export const DraggingContext = createContext(false); -export const DropzoneContext = createContext<[DropzoneSet, Dispatch<Action>]>([ - {}, - () => {}, -]); const StyledDiv = styled.div` display: flex; flex-direction: column; @@ -45,53 +34,10 @@ const StyledDiv = styled.div` min-height: 0; `; -const reducer = (state: DropzoneSet = {}, action: Action) => { - if (action.canDrop) { - return { - ...state, - [action.key]: action.canDrop, - }; - } - if (action.key) { - const newState = { ...state }; - delete newState[action.key]; - return newState; - } - return state; -}; - -const ExploreContainer: FC<{}> = ({ children }) => { - const dragDropManager = useDragDropManager(); - const [dragging, setDragging] = useState( - dragDropManager.getMonitor().isDragging(), - ); - - useEffect(() => { - const monitor = dragDropManager.getMonitor(); - const unsub = monitor.subscribeToStateChange(() => { - const item = monitor.getItem() || {}; - // don't show dragging state for the sorting item - if ('dragIndex' in item) { - return; - } - const isDragging = monitor.isDragging(); - setDragging(isDragging); - }); - - return () => { - unsub(); - }; - }, [dragDropManager]); - - const dropzoneValue = useReducer(reducer, {}); - - return ( - <DropzoneContext.Provider value={dropzoneValue}> - <DraggingContext.Provider value={dragging}> - <StyledDiv>{children}</StyledDiv> - </DraggingContext.Provider> - </DropzoneContext.Provider> - ); -}; +const ExploreContainer: FC<{}> = ({ children }) => ( + <ExploreDndContextProvider> + <StyledDiv>{children}</StyledDiv> + </ExploreDndContextProvider> +); export default ExploreContainer; diff --git a/superset-frontend/src/explore/components/controls/ContourControl/index.tsx b/superset-frontend/src/explore/components/controls/ContourControl/index.tsx index c8148071c91..16f28b3f8b4 100644 --- a/superset-frontend/src/explore/components/controls/ContourControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/ContourControl/index.tsx @@ -127,6 +127,8 @@ const ContourControl = ({ onChange, ...props }: ContourControlProps) => { accept={[]} ghostButtonText={ghostButtonText} onClickGhostButton={handleClickGhostButton} + sortableType="ContourOption" + itemCount={contours.length} {...props} /> <ContourPopoverTrigger diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx index 6f560d1ff19..4fc6b4c2beb 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx @@ -185,6 +185,9 @@ function DndColumnSelect(props: DndColumnSelectProps) { [ghostButtonText, multi], ); + // Generate sortable type that matches OptionWrapper's type + const sortableType = `${DndItemType.ColumnOption}_${name}_${label}`; + return ( <div> <DndSelectLabel @@ -195,6 +198,8 @@ function DndColumnSelect(props: DndColumnSelectProps) { displayGhostButton={multi || optionSelector.values.length === 0} ghostButtonText={labelGhostButtonText} onClickGhostButton={openPopover} + sortableType={sortableType} + itemCount={optionSelector.values.length} {...props} /> <ColumnSelectPopoverTrigger diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx index 9b1a9a371df..b3447ffd817 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx @@ -452,6 +452,8 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { accept={DND_ACCEPTED_TYPES} ghostButtonText={t('Drop columns/metrics here or click')} onClickGhostButton={handleClickGhostButton} + sortableType={DndItemType.FilterOption} + itemCount={values.length} {...props} /> <AdhocFilterPopoverTrigger diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx index aefdc781cdf..8d0db502482 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx @@ -377,6 +377,9 @@ const DndMetricSelect = (props: any) => { multi ? 2 : 1, ); + // Generate sortable type that matches MetricDefinitionValue's type + const sortableType = `${DndItemType.AdhocMetricOption}_${props.name}_${props.label}`; + return ( <div className="metrics-select"> <DndSelectLabel @@ -387,6 +390,8 @@ const DndMetricSelect = (props: any) => { ghostButtonText={ghostButtonText} displayGhostButton={multi || value.length === 0} onClickGhostButton={handleClickGhostButton} + sortableType={sortableType} + itemCount={value.length} {...props} /> <AdhocMetricPopoverTrigger diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.test.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.test.tsx index 5df594ccb71..515c3e9ea33 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.test.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.test.tsx @@ -52,7 +52,7 @@ const MockChildren = () => { }; test('renders with default props', () => { - render(<DndSelectLabel {...defaultProps} />, { useDnd: true }); + render(<DndSelectLabel {...defaultProps} />, { useDndKit: true }); expect(screen.getByText('Drop columns here or click')).toBeInTheDocument(); }); @@ -60,7 +60,7 @@ test('renders ghost button when empty', () => { const ghostButtonText = 'Ghost button text'; render( <DndSelectLabel {...defaultProps} ghostButtonText={ghostButtonText} />, - { useDnd: true }, + { useDndKit: true }, ); expect(screen.getByText(ghostButtonText)).toBeInTheDocument(); }); @@ -69,13 +69,13 @@ test('renders values', () => { const values = 'Values'; const valuesRenderer = () => <span>{values}</span>; render(<DndSelectLabel {...defaultProps} valuesRenderer={valuesRenderer} />, { - useDnd: true, + useDndKit: true, }); expect(screen.getByText(values)).toBeInTheDocument(); }); test('Handles ghost button click', () => { - render(<DndSelectLabel {...defaultProps} />, { useDnd: true }); + render(<DndSelectLabel {...defaultProps} />, { useDndKit: true }); userEvent.click(screen.getByText('Drop columns here or click')); expect(defaultProps.onClickGhostButton).toHaveBeenCalled(); }); @@ -86,7 +86,6 @@ test('updates dropValidator on changes', () => { <DndSelectLabel {...defaultProps} /> <MockChildren /> </ExploreContainer>, - { useDnd: true }, ); expect(getByTestId(`mock-result-${defaultProps.name}`)).toHaveTextContent( 'false', diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx index 153d97d6384..fcdd4f227b1 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx @@ -17,7 +17,11 @@ * under the License. */ import { ReactNode, useCallback, useContext, useEffect, useMemo } from 'react'; -import { useDrop } from 'react-dnd'; +import { useDroppable } from '@dnd-kit/core'; +import { + SortableContext, + verticalListSortingStrategy, +} from '@dnd-kit/sortable'; import { t } from '@apache-superset/core'; import ControlHeader from 'src/explore/components/ControlHeader'; import { @@ -45,6 +49,9 @@ export type DndSelectLabelProps = { displayGhostButton?: boolean; onClickGhostButton: () => void; isLoading?: boolean; + // For sortable items - the type string and count to generate sortable IDs + sortableType?: string; + itemCount?: number; }; export default function DndSelectLabel({ @@ -52,34 +59,45 @@ export default function DndSelectLabel({ accept, valuesRenderer, isLoading, + sortableType, + itemCount = 0, ...props }: DndSelectLabelProps) { const canDropProp = props.canDrop; const canDropValueProp = props.canDropValue; + const acceptTypes = useMemo( + () => (Array.isArray(accept) ? accept : [accept]), + [accept], + ); + const dropValidator = useCallback( (item: DatasourcePanelDndItem) => canDropProp(item) && (canDropValueProp?.(item.value) ?? true), [canDropProp, canDropValueProp], ); - const [{ isOver, canDrop }, datasourcePanelDrop] = useDrop({ - accept: isLoading ? [] : accept, - - drop: (item: DatasourcePanelDndItem) => { - props.onDrop(item); - props.onDropValue?.(item.value); + const { setNodeRef, isOver, active } = useDroppable({ + id: `dropzone-${props.name}`, + disabled: isLoading, + data: { + accept: acceptTypes, + onDrop: props.onDrop, + onDropValue: props.onDropValue, }, - - canDrop: dropValidator, - - collect: monitor => ({ - isOver: monitor.isOver(), - canDrop: monitor.canDrop(), - type: monitor.getItemType(), - }), }); + // Check if the active dragged item can be dropped here + const canDrop = useMemo(() => { + if (!active?.data.current) return false; + const activeData = active.data.current as { type: string; value: unknown }; + if (!acceptTypes.includes(activeData.type as DndItemType)) return false; + return dropValidator({ + type: activeData.type as DndItemType, + value: activeData.value as DndItemValue, + }); + }, [active, acceptTypes, dropValidator]); + const dispatch = useContext(DropzoneContext)[1]; useEffect(() => { @@ -93,6 +111,15 @@ export default function DndSelectLabel({ const values = useMemo(() => valuesRenderer(), [valuesRenderer]); + // Generate sortable item IDs for SortableContext + const sortableItemIds = useMemo(() => { + if (!sortableType || itemCount === 0) return []; + return Array.from( + { length: itemCount }, + (_, i) => `sortable-${sortableType}-${i}`, + ); + }, [sortableType, itemCount]); + function renderGhostButton() { return ( <AddControlLabel @@ -105,8 +132,31 @@ export default function DndSelectLabel({ ); } + // Handle drop events from dnd-kit + useEffect(() => { + if (isOver && active?.data.current && canDrop) { + // The actual drop is handled in ExploreDndContext's onDragEnd + // This effect is for any side effects needed during hover + } + }, [isOver, active, canDrop]); + + // Wrap values in SortableContext if sortable + const renderSortableValues = () => { + if (sortableItemIds.length > 0) { + return ( + <SortableContext + items={sortableItemIds} + strategy={verticalListSortingStrategy} + > + {values} + </SortableContext> + ); + } + return values; + }; + return ( - <div ref={datasourcePanelDrop}> + <div ref={setNodeRef}> <HeaderContainer> <ControlHeader {...props} /> </HeaderContainer> @@ -117,7 +167,7 @@ export default function DndSelectLabel({ isDragging={isDragging} isLoading={isLoading} > - {values} + {renderSortableValues()} {displayGhostButton && renderGhostButton()} </DndLabelsContainer> </div> diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx index 3d1a5a9d785..519cf3e5741 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { render, screen, fireEvent } from 'spec/helpers/testing-library'; +import { render, screen } from 'spec/helpers/testing-library'; import { DndItemType } from 'src/explore/components/DndItemType'; import OptionWrapper from 'src/explore/components/controls/DndColumnSelectControl/OptionWrapper'; @@ -29,35 +29,66 @@ test('renders with default props', async () => { onShiftOptions={jest.fn()} label="Option" />, - { useDnd: true }, + { useDndKit: true }, ); expect(container).toBeInTheDocument(); expect(await screen.findByRole('img', { name: 'close' })).toBeInTheDocument(); }); -test('triggers onShiftOptions on drop', async () => { - const onShiftOptions = jest.fn(); +test('renders label correctly', async () => { + render( + <OptionWrapper + index={1} + clickClose={jest.fn()} + type={'Column' as DndItemType} + onShiftOptions={jest.fn()} + label="Test Label" + />, + { useDndKit: true }, + ); + + expect(await screen.findByText('Test Label')).toBeInTheDocument(); +}); + +test('renders multiple options', async () => { render( <> <OptionWrapper - index={1} + index={0} clickClose={jest.fn()} type={'Column' as DndItemType} - onShiftOptions={onShiftOptions} + onShiftOptions={jest.fn()} label="Option 1" /> <OptionWrapper - index={2} + index={1} clickClose={jest.fn()} type={'Column' as DndItemType} - onShiftOptions={onShiftOptions} + onShiftOptions={jest.fn()} label="Option 2" /> </>, - { useDnd: true }, + { useDndKit: true }, + ); + + expect(await screen.findByText('Option 1')).toBeInTheDocument(); + expect(await screen.findByText('Option 2')).toBeInTheDocument(); +}); + +test('calls clickClose when close button is clicked', async () => { + const clickClose = jest.fn(); + render( + <OptionWrapper + index={1} + clickClose={clickClose} + type={'Column' as DndItemType} + onShiftOptions={jest.fn()} + label="Option" + />, + { useDndKit: true }, ); - fireEvent.dragStart(await screen.findByText('Option 1')); - fireEvent.drop(await screen.findByText('Option 2')); - expect(onShiftOptions).toHaveBeenCalled(); + const closeButton = await screen.findByRole('img', { name: 'close' }); + closeButton.click(); + expect(clickClose).toHaveBeenCalledWith(1); }); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx index a34046f9d5e..fd06b2105d7 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.tsx @@ -16,13 +16,9 @@ * specific language governing permissions and limitations * under the License. */ -import { useRef } from 'react'; -import { - useDrag, - useDrop, - DropTargetMonitor, - DragSourceMonitor, -} from 'react-dnd'; +import { useRef, useMemo } from 'react'; +import { useSortable } from '@dnd-kit/sortable'; +import { CSS } from '@dnd-kit/utilities'; import { DragContainer } from 'src/explore/components/controls/OptionControls'; import { OptionProps, @@ -64,62 +60,32 @@ export default function OptionWrapper( multiValueWarningMessage, ...rest } = props; - const ref = useRef<HTMLDivElement>(null); const labelRef = useRef<HTMLDivElement>(null); - const [{ isDragging }, drag] = useDrag({ - item: { + // Create a unique sortable ID for this item + const sortableId = useMemo(() => `sortable-${type}-${index}`, [type, index]); + + const { + attributes, + listeners, + setNodeRef, + transform, + transition, + isDragging, + } = useSortable({ + id: sortableId, + data: { type, dragIndex: index, - }, - collect: (monitor: DragSourceMonitor) => ({ - isDragging: monitor.isDragging(), - }), + onShiftOptions, + } as OptionItemInterface & { onShiftOptions: typeof onShiftOptions }, }); - const [, drop] = useDrop({ - accept: type, - - hover: (item: OptionItemInterface, monitor: DropTargetMonitor) => { - if (!ref.current) { - return; - } - const { dragIndex } = item; - const hoverIndex = index; - - // Don't replace items with themselves - if (dragIndex === hoverIndex) { - return; - } - // Determine rectangle on screen - const hoverBoundingRect = ref.current?.getBoundingClientRect(); - // Get vertical middle - const hoverMiddleY = - (hoverBoundingRect.bottom - hoverBoundingRect.top) / 2; - // Determine mouse position - const clientOffset = monitor.getClientOffset(); - // Get pixels to the top - const hoverClientY = clientOffset - ? clientOffset.y - hoverBoundingRect.top - : 0; - // Only perform the move when the mouse has crossed half of the items height - // When dragging downwards, only move when the cursor is below 50% - // When dragging upwards, only move when the cursor is above 50% - // Dragging downwards - if (dragIndex < hoverIndex && hoverClientY < hoverMiddleY) { - return; - } - // Dragging upwards - if (dragIndex > hoverIndex && hoverClientY > hoverMiddleY) { - return; - } - - // Time to actually perform the action - onShiftOptions(dragIndex, hoverIndex); - // eslint-disable-next-line no-param-reassign - item.dragIndex = hoverIndex; - }, - }); + const style = { + transform: CSS.Transform.toString(transform), + transition, + opacity: isDragging ? 0.5 : 1, + }; const shouldShowTooltip = (!isDragging && tooltipTitle && label && tooltipTitle !== label) || @@ -179,10 +145,14 @@ export default function OptionWrapper( return null; }; - drag(drop(ref)); - return ( - <DragContainer ref={ref} {...rest}> + <DragContainer + ref={setNodeRef} + style={style} + {...attributes} + {...listeners} + {...rest} + > <Option index={index} clickClose={clickClose} diff --git a/superset-frontend/src/explore/components/controls/OptionControls/OptionControls.test.tsx b/superset-frontend/src/explore/components/controls/OptionControls/OptionControls.test.tsx index 2f42a8c9d5b..119409c3beb 100644 --- a/superset-frontend/src/explore/components/controls/OptionControls/OptionControls.test.tsx +++ b/superset-frontend/src/explore/components/controls/OptionControls/OptionControls.test.tsx @@ -16,12 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { - render, - screen, - fireEvent, - waitFor, -} from 'spec/helpers/testing-library'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; import { OptionControlLabel, DragContainer, @@ -48,7 +43,7 @@ const defaultProps = { const setup = (overrides?: Record<string, any>) => render(<OptionControlLabel {...defaultProps} {...overrides} />, { - useDnd: true, + useDndKit: true, }); test('should render', async () => { @@ -88,7 +83,7 @@ test('should display a certification icon if saved metric is certified', async ( ); }); -test('triggers onMoveLabel on drop', async () => { +test('renders multiple labels', async () => { render( <> <OptionControlLabel @@ -101,15 +96,11 @@ test('triggers onMoveLabel on drop', async () => { index={2} label={<span>Label 2</span>} /> - , </>, - { useDnd: true }, + { useDndKit: true }, ); - await waitFor(() => { - fireEvent.dragStart(screen.getByText('Label 1')); - fireEvent.drop(screen.getByText('Label 2')); - expect(defaultProps.onMoveLabel).toHaveBeenCalled(); - }); + expect(await screen.findByText('Label 1')).toBeInTheDocument(); + expect(await screen.findByText('Label 2')).toBeInTheDocument(); }); test('renders DragContainer', () => { diff --git a/superset-frontend/src/explore/components/controls/OptionControls/index.tsx b/superset-frontend/src/explore/components/controls/OptionControls/index.tsx index 03c6a3d30a9..12ced26352b 100644 --- a/superset-frontend/src/explore/components/controls/OptionControls/index.tsx +++ b/superset-frontend/src/explore/components/controls/OptionControls/index.tsx @@ -16,9 +16,9 @@ * specific language governing permissions and limitations * under the License. */ -import { useRef, ReactNode } from 'react'; - -import { useDrag, useDrop, DropTargetMonitor } from 'react-dnd'; +import { useRef, ReactNode, useMemo } from 'react'; +import { useSortable } from '@dnd-kit/sortable'; +import { CSS } from '@dnd-kit/utilities'; import { t } from '@apache-superset/core'; import { styled, useTheme, css, keyframes } from '@apache-superset/core/ui'; import { InfoTooltip, Icons, Tooltip } from '@superset-ui/core/components'; @@ -233,9 +233,12 @@ export const AddIconButton = styled.button` } `; -interface DragItem { - dragIndex: number; +export interface SortableItemData { type: string; + dragIndex: number; + onMoveLabel?: (dragIndex: number, hoverIndex: number) => void; + onDropLabel?: () => void; + value?: savedMetricType | AdhocMetric; } export const OptionControlLabel = ({ @@ -272,73 +275,37 @@ export const OptionControlLabel = ({ multi?: boolean; }) => { const theme = useTheme(); - const ref = useRef<HTMLDivElement>(null); const labelRef = useRef<HTMLDivElement>(null); const hasMetricName = savedMetric?.metric_name; - const [, drop] = useDrop({ - accept: type, - drop() { - if (!multi) { - return; - } - onDropLabel?.(); - }, - hover(item: DragItem, monitor: DropTargetMonitor) { - if (!multi) { - return; - } - if (!ref.current) { - return; - } - const { dragIndex } = item; - const hoverIndex = index; - // Don't replace items with themselves - if (dragIndex === hoverIndex) { - return; - } - // Determine rectangle on screen - const hoverBoundingRect = ref.current?.getBoundingClientRect(); - // Get vertical middle - const hoverMiddleY = - (hoverBoundingRect.bottom - hoverBoundingRect.top) / 2; - // Determine mouse position - const clientOffset = monitor.getClientOffset(); - // Get pixels to the top - const hoverClientY = clientOffset?.y - ? clientOffset?.y - hoverBoundingRect.top - : 0; - // Only perform the move when the mouse has crossed half of the items height - // When dragging downwards, only move when the cursor is below 50% - // When dragging upwards, only move when the cursor is above 50% - // Dragging downwards - if (dragIndex < hoverIndex && hoverClientY < hoverMiddleY) { - return; - } - // Dragging upwards - if (dragIndex > hoverIndex && hoverClientY > hoverMiddleY) { - return; - } - // Time to actually perform the action - onMoveLabel?.(dragIndex, hoverIndex); - // Note: we're mutating the monitor item here! - // Generally it's better to avoid mutations, - // but it's good here for the sake of performance - // to avoid expensive index searches. - // eslint-disable-next-line no-param-reassign - item.dragIndex = hoverIndex; - }, - }); - const [{ isDragging }, drag] = useDrag({ - item: { + + // Create a unique sortable ID for this item + const sortableId = useMemo(() => `sortable-${type}-${index}`, [type, index]); + + const { + attributes, + listeners, + setNodeRef, + transform, + transition, + isDragging, + } = useSortable({ + id: sortableId, + disabled: !multi, + data: { type, dragIndex: index, + onMoveLabel, + onDropLabel, value: savedMetric?.metric_name ? savedMetric : adhocMetric, - }, - collect: monitor => ({ - isDragging: monitor.isDragging(), - }), + } as SortableItemData, }); + const style = { + transform: CSS.Transform.toString(transform), + transition, + opacity: isDragging ? 0.5 : 1, + }; + const getLabelContent = () => { const shouldShowTooltip = (!isDragging && @@ -423,6 +390,14 @@ export const OptionControlLabel = ({ </OptionControlContainer> ); - drag(drop(ref)); - return <DragContainer ref={ref}>{getOptionControlContent()}</DragContainer>; + return ( + <DragContainer + ref={setNodeRef} + style={style} + {...attributes} + {...listeners} + > + {getOptionControlContent()} + </DragContainer> + ); };
