rusackas commented on code in PR #37880: URL: https://github.com/apache/superset/pull/37880#discussion_r3365258193
########## 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); Review Comment: Good catch, and you were right that the prior fix was incomplete. Fixed in c9bc94648e. Two things: 1. I now read the handlers from `over.data.current` in the drag-end dispatcher (no registry lookup), and I went further than the type check: react-dnd only fired `drop` when **both** `accept` matched **and** `canDrop` returned true, so I exposed each droppable's `canDrop` validator in its `useDroppable` data and the dispatcher now honors it. Without that, duplicates and non-selected metrics would have been droppable — a regression the type-only check would have introduced. 2. I extracted the whole drag-end decision into a pure exported `resolveDragEnd(active, over)` so the behavior is unit-testable without driving `PointerSensor` (jsdom can't), and removed the now-unused `DropHandlersContext` register/unregister registry entirely (grepped the repo first — nothing else referenced it). Verified via new handler-level + component tests (drop fires, dupes/non-selected rejected, reorder works) and reasoned through parity with the original react-dnd `drop`/`canDrop` semantics. ########## superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx: ########## @@ -45,41 +49,55 @@ 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({ displayGhostButton = true, 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, Review Comment: Fixed in c9bc94648e. `DndSelectLabel` was indeed the right instinct — storing the callbacks in the `useDroppable` data. The missing half was on the `ExploreDndContext` side, which I rewired to read `over.data.current.onDrop`/`onDropValue` instead of the dead registry. I also added `canDrop` to the droppable data here so the dispatcher can reproduce react-dnd's gating (it only fired `drop` when `canDrop` passed). See the full trace/fix on the ExploreDndContext thread. -- 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]
