Copilot commented on code in PR #36301:
URL: https://github.com/apache/superset/pull/36301#discussion_r2578301388
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx:
##########
@@ -18,24 +18,19 @@
*/
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';
Review Comment:
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.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx:
##########
@@ -57,91 +52,31 @@ interface FilterTabTitleProps {
index: number;
filterIds: string[];
onRearrange: (dragItemIndex: number, targetIndex: number) => void;
Review Comment:
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.
```suggestion
filterIds: string[];
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx:
##########
@@ -57,91 +52,31 @@ interface FilterTabTitleProps {
index: number;
filterIds: string[];
onRearrange: (dragItemIndex: number, targetIndex: number) => void;
- children?: React.ReactNode;
-}
-
-interface DragItem {
- index: number;
- filterIds: string[];
- type: string;
}
export const DraggableFilter: FC<FilterTabTitleProps> = ({
- index,
- onRearrange,
filterIds,
children,
}) => {
- const ref = useRef<HTMLDivElement>(null);
- const [{ isDragging }, drag] = useDrag({
- type: FILTER_TYPE,
- item: { filterIds, type: FILTER_TYPE, index },
- collect: (monitor: DragSourceMonitor) => ({
- isDragging: monitor.isDragging(),
- }),
- });
- const [, drop] = useDrop({
- accept: FILTER_TYPE,
- hover: (item: DragItem, monitor: DropTargetMonitor) => {
- if (!ref.current) {
- return;
- }
-
- const dragIndex = item.index;
- 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 as XYCoord).y - hoverBoundingRect.top;
-
- // 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;
- }
+ const { attributes, listeners, setNodeRef, isDragging, transform, transition
} = useSortable({ id: filterIds[0] });
- // Dragging upwards
- if (dragIndex > hoverIndex && hoverClientY > hoverMiddleY) {
- return;
- }
+ const style = {
+ transform: CSS.Transform.toString(transform),
+ transition,
+ };
- onRearrange(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.index = hoverIndex;
- },
- });
- drag(drop(ref));
return (
- <Container ref={ref} isDragging={isDragging}>
- <DragIcon
- isDragging={isDragging}
- alt="Move icon"
- className="dragIcon"
- viewBox="4 4 16 16"
- />
- <div css={{ flex: 1 }}>{children}</div>
+ <Container ref={setNodeRef} style={style} isDragging={isDragging}
{...listeners} {...attributes}>
+ <DragIcon
+ isDragging={isDragging}
+ alt="Move icon"
+ className="dragIcon"
+ viewBox="4 4 16 16"
+ />
+ <div css={{ flex: 1 }}>{children}</div>
</Container>
- );
+ )
Review Comment:
Missing semicolon at the end of the return statement. Add a semicolon after
the closing parenthesis for consistency with the rest of the codebase.
```suggestion
);
```
--
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]
