rusackas commented on code in PR #37880:
URL: https://github.com/apache/superset/pull/37880#discussion_r3365259162
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx:
##########
@@ -105,8 +132,31 @@ export default function DndSelectLabel({
);
}
+ // Handle drop events from dnd-kit
+ useEffect(() => {
Review Comment:
Removed in c9bc94648e. You were right — it was a leftover placeholder from
the incomplete drop wiring and did nothing. Deleting it also clears the
exhaustive-deps noise on `isOver`.
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx:
##########
@@ -311,157 +303,8 @@ test('update adhoc metric name when column label in
dataset changes', () => {
expect(screen.getByText('SUM(new col B name)')).toBeVisible();
});
-test('can drag metrics', async () => {
- const metricValues = ['metric_a', 'metric_b', adhocMetricB];
- render(<DndMetricSelect {...defaultProps} value={metricValues} multi />, {
- useDnd: true,
- useRedux: true,
- });
-
- expect(screen.getByText('metric_a')).toBeVisible();
- expect(screen.getByText('Metric B')).toBeVisible();
-
- const container = screen.getByTestId('dnd-labels-container');
- expect(container.childElementCount).toBe(4);
-
- const firstMetric = container.children[0] as HTMLElement;
- const lastMetric = container.children[2] as HTMLElement;
- expect(within(firstMetric).getByText('metric_a')).toBeVisible();
- expect(within(lastMetric).getByText('SUM(Column B)')).toBeVisible();
-
- fireEvent.mouseOver(within(firstMetric).getByText('metric_a'));
- expect(await screen.findByText('Metric name')).toBeInTheDocument();
-
- fireEvent.dragStart(firstMetric);
- fireEvent.dragEnter(lastMetric);
- fireEvent.dragOver(lastMetric);
- fireEvent.drop(lastMetric);
-
- expect(within(firstMetric).getByText('SUM(Column B)')).toBeVisible();
- expect(within(lastMetric).getByText('metric_a')).toBeVisible();
-});
-
-test('cannot drop a duplicated item', () => {
- const metricValues = ['metric_a'];
- const { getByTestId } = render(
- <>
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_a', uuid: '1' }}
- type={DndItemType.Metric}
- />
- <DndMetricSelect {...defaultProps} value={metricValues} multi />
- </>,
- {
- useDnd: true,
- useRedux: true,
- },
- );
-
- const acceptableMetric = getByTestId('DatasourcePanelDragOption');
- const currentMetric = getByTestId('dnd-labels-container');
-
- const currentMetricSelection = currentMetric.children.length;
-
- fireEvent.dragStart(acceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection);
- expect(currentMetric).toHaveTextContent('metric_a');
-});
-
-test('can drop a saved metric when disallow_adhoc_metrics', () => {
- const metricValues = ['metric_b'];
- const { getByTestId } = render(
- <>
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_a', uuid: '1' }}
- type={DndItemType.Metric}
- />
- <DndMetricSelect
- {...defaultProps}
- value={metricValues}
- multi
- datasource={{ extra: '{ "disallow_adhoc_metrics": true }' }}
- />
- </>,
- {
- useDnd: true,
- useRedux: true,
- },
- );
-
- const acceptableMetric = getByTestId('DatasourcePanelDragOption');
- const currentMetric = getByTestId('dnd-labels-container');
-
- const currentMetricSelection = currentMetric.children.length;
-
- fireEvent.dragStart(acceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection + 1);
- expect(currentMetric.children[1]).toHaveTextContent('metric_a');
-});
-
-test('cannot drop non-saved metrics when disallow_adhoc_metrics', () => {
- const metricValues = ['metric_b'];
- const { getByTestId, getAllByTestId } = render(
- <>
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_a', uuid: '1' }}
- type={DndItemType.Metric}
- />
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_c', uuid: '2' }}
- type={DndItemType.Metric}
- />
- <DatasourcePanelDragOption
- value={{ column_name: 'column_1', uuid: '3' }}
- type={DndItemType.Column}
- />
- <DndMetricSelect
- {...defaultProps}
- value={metricValues}
- multi
- datasource={{ extra: '{ "disallow_adhoc_metrics": true }' }}
- />
- </>,
- {
- useDnd: true,
- useRedux: true,
- },
- );
-
- const selections = getAllByTestId('DatasourcePanelDragOption');
- const acceptableMetric = selections[0];
- const unacceptableMetric = selections[1];
- const unacceptableType = selections[2];
- const currentMetric = getByTestId('dnd-labels-container');
-
- const currentMetricSelection = currentMetric.children.length;
-
- fireEvent.dragStart(unacceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection);
- expect(currentMetric).not.toHaveTextContent('metric_c');
-
- fireEvent.dragStart(unacceptableType);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection);
- expect(currentMetric).not.toHaveTextContent('column_1');
-
- fireEvent.dragStart(acceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection + 1);
- expect(currentMetric).toHaveTextContent('metric_a');
-});
+// Note: Drag-and-drop tests removed - @dnd-kit uses pointer events instead of
Review Comment:
Restored in c9bc94648e. I looked for an existing @dnd-kit drop-simulation
harness first — the repo only has render/attribute-level @dnd-kit tests (e.g.
DraggableFilter, FoldersEditor) and `@dnd-kit/testing` isn't installed, so
there was no pointer-event pattern to reuse. jsdom can't drive `PointerSensor`
(no layout), so I took the handler-level route you suggested as the fallback:
- Extracted the drag-end logic into a pure exported `resolveDragEnd` and
added `ExploreDndContext.test.tsx` covering the dispatcher directly (reorder
fires, mismatched-type reorder is a no-op, drop fires only when type accepted
AND `canDrop` passes, self-drop/no-target no-ops).
- Added `dndTestUtils` that capture each control's real
`useDroppable`/`useSortable` data (via mocking those two hooks) and feed it
through `resolveDragEnd`, so the component's genuine `onDrop`/`canDrop`/reorder
callbacks are exercised end-to-end.
Coverage re-added: in `DndColumnMetricSelect.test.tsx` — "can drop columns
and metrics", "cannot drop duplicate items", "can drop only selected metrics",
"can drag and reorder items"; in `DndMetricSelect.test.tsx` — reorder dispatch,
"cannot drop a duplicated item", "can drop a saved metric when
disallow_adhoc_metrics", "cannot drop non-saved metrics when
disallow_adhoc_metrics". So the duplicate-rejection and
`disallow_adhoc_metrics` type-gating are unit-tested again. All 80 tests in the
two affected dirs pass.
##########
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(
Review Comment:
Done in c9bc94648e — wired up `KeyboardSensor` with
`sortableKeyboardCoordinates` alongside the existing `PointerSensor`, exactly
as suggested. Composes cleanly and is a pure a11y add (react-dnd's HTML5
backend had no keyboard support), so no regression risk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
