Fix(1-3462)/janky drag n drop (#9599)

Fixes janky drag and drop behavior and updates the styling of the drag handle focus. The solution uses the same method to prevent oscillation as we do for strategies. To get access to the same context, I've added some extra parameters to the OnMoveItem function and passed along the extra data from the `useDragItem` hook. No new information, just making more of it available, and turning it into an object so that you can declare the properties you need (and get rid of potential wrong ordering of drag/drop indices). For the drag and drop behavior: If the dragged element is the same size or smaller than the element you're dragging over, they will swap places as soon as you enter that space. If the target element is larger, however, they won't swap until you reach the drag/drop handle, even if they could theoretically switch somewhere in the middle. This appears to be a limitation of how the drag/drop event system works. New drag events are only fired when you "dragenter" a new element, so it never fires anywhere in the middle. Technically, we could insert more empty spans inside the drag handle to trigger more events, but I wanna hold off on that because it doesn't sound great. When dragging, only the handle is visible; the rest of the card stays in place. For strategies, we show a "ghost" version of the config you're dragging. However, if you apply the drag handle to the card itself, all of it becomes draggable, but you can no longer select the text inside it, which is unfortunate. Strategies do solev this, though, but I haven't been able to figure out why. If you know, please share! Before: ![image](https://github.com/user-attachments/assets/d3bf9fd2-7d74-4ad7-adde-b729403f82b3) After: ![image](https://github.com/user-attachments/assets/27d3ca4e-112a-4420-b10d-7df59a7c09a0)
2025-07-26 13:48:33 +02:00 · 2025-03-25 11:43:40 +01:00 · 2025-03-25 11:43:40 +01:00 · 3d1a97f745
commit 3d1a97f745
parent d8c7e31b18
6 changed files with 115 additions and 84 deletions
--- a/frontend/src/component/environments/EnvironmentTable/EnvironmentTable.tsx
+++ b/frontend/src/component/environments/EnvironmentTable/EnvironmentTable.tsx
@ -39,7 +39,7 @@ export const EnvironmentTable = () => {
    const isFeatureEnabled = useUiFlag('EEA');

    const onMoveItem: OnMoveItem = useCallback(
-        async (dragIndex: number, dropIndex: number, save = false) => {
+        async ({ dragIndex, dropIndex, save }) => {
            const oldEnvironments = environments || [];
            const newEnvironments = [...oldEnvironments];
            const movedEnvironment = newEnvironments.splice(dragIndex, 1)[0];
--- a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/ProjectEnvironmentStrategyDraggableItem.tsx
+++ b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/ProjectEnvironmentStrategyDraggableItem.tsx
@ -17,7 +17,6 @@ import { UPDATE_FEATURE_STRATEGY } from '@server/types/permissions';
 import { StrategyDraggableItem } from './StrategyDraggableItem';

 type ProjectEnvironmentStrategyDraggableItemProps = {
-    className?: string;
    strategy: IFeatureStrategy;
    environmentName: string;
    index: number;
@ -35,7 +34,6 @@ type ProjectEnvironmentStrategyDraggableItemProps = {
 };

 export const ProjectEnvironmentStrategyDraggableItem = ({
-    className,
    strategy,
    index,
    environmentName,
--- a/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCard.tsx
+++ b/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCard.tsx
@ -10,13 +10,12 @@ import {
    FormHelperText,
 } from '@mui/material';
 import type { IReleasePlanMilestoneStrategy } from 'interfaces/releasePlans';
-import { type DragEventHandler, type RefObject, useRef, useState } from 'react';
+import { type DragEventHandler, type RefObject, useState } from 'react';
 import ExpandMore from '@mui/icons-material/ExpandMore';
 import { MilestoneCardName } from './MilestoneCardName';
 import { MilestoneStrategyMenuCards } from './MilestoneStrategyMenu/MilestoneStrategyMenuCards';
 import { SidebarModal } from 'component/common/SidebarModal/SidebarModal';
 import { ReleasePlanTemplateAddStrategyForm } from '../../MilestoneStrategy/ReleasePlanTemplateAddStrategyForm';
-import DragIndicator from '@mui/icons-material/DragIndicator';
 import { type OnMoveItem, useDragItem } from 'hooks/useDragItem';
 import type { IExtendedMilestonePayload } from 'component/releases/hooks/useTemplateForm';

@ -24,9 +23,9 @@ import { StrategySeparator } from 'component/common/StrategySeparator/StrategySe
 import Edit from '@mui/icons-material/Edit';
 import Delete from '@mui/icons-material/DeleteOutlined';
 import { StrategyDraggableItem } from 'component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyDraggableItem';
-import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly';
 import { StrategyList } from 'component/common/StrategyList/StrategyList';
 import { StrategyListItem } from 'component/common/StrategyList/StrategyListItem';
+import { MilestoneCardDragHandle } from './MilestoneCardDragHandle';

 const leftPadding = 3;

@ -122,27 +121,6 @@ const StyledIconButton = styled(IconButton)(({ theme }) => ({
    color: theme.palette.primary.main,
 }));

-const DragButton = styled('button')(({ theme }) => ({
-    padding: 0,
-    cursor: 'grab',
-    transition: 'background-color 0.2s ease-in-out',
-    backgroundColor: 'inherit',
-    border: 'none',
-    borderRadius: theme.shape.borderRadiusMedium,
-    color: theme.palette.text.secondary,
-    '&:hover, &:focus-visible': {
-        background: theme.palette.table.headerHover,
-        outline: 'none',
-    },
-}));
-
-const DraggableContent = styled('span')(({ theme }) => ({
-    paddingTop: theme.spacing(2.75),
-    display: 'block',
-    height: '100%',
-    width: '100%',
-}));
-
 export interface IMilestoneCardProps {
    milestone: IExtendedMilestonePayload;
    milestoneChanged: (milestone: IExtendedMilestonePayload) => void;
@ -178,22 +156,7 @@ export const MilestoneCard = ({
        ? 'MilestoneStrategyMenuPopover'
        : undefined;

-    const dragHandleRef = useRef(null);
-
-    const dragItemRef = useDragItem<HTMLTableRowElement>(
-        index,
-        onMoveItem,
-        dragHandleRef,
-    );
-
-    const dragHandle = (
-        <DragButton type='button'>
-            <DraggableContent ref={dragItemRef}>
-                <DragIndicator aria-hidden />
-                <ScreenReaderOnly>Drag to reorder</ScreenReaderOnly>
-            </DraggableContent>
-        </DragButton>
-    );
+    const dragItemRef = useDragItem<HTMLSpanElement>(index, onMoveItem);

    const onClose = () => {
        setAnchor(undefined);
@ -363,7 +326,7 @@ export const MilestoneCard = ({
        return (
            <>
                <DraggableCardContainer>
-                    {dragHandle}
+                    <MilestoneCardDragHandle dragItemRef={dragItemRef} />
                    <StyledMilestoneCard
                        hasError={
                            Boolean(errors?.[milestone.id]) ||
@ -445,8 +408,8 @@ export const MilestoneCard = ({

    return (
        <>
-            <DraggableCardContainer ref={dragItemRef}>
-                {dragHandle}
+            <DraggableCardContainer>
+                <MilestoneCardDragHandle dragItemRef={dragItemRef} />
                <StyledAccordion
                    expanded={expanded}
                    onChange={(e, change) => setExpanded(change)}
--- a/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCardDragHandle.tsx
+++ b/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCardDragHandle.tsx
@ -0,0 +1,54 @@
+import DragIndicator from '@mui/icons-material/DragIndicator';
+import { styled } from '@mui/material';
+import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly';
+import type { FC } from 'react';
+
+const DragButton = styled('button')(({ theme }) => ({
+    padding: 0,
+    cursor: 'grab',
+    transition: 'background-color 0.2s ease-in-out',
+    backgroundColor: 'inherit',
+    border: 'none',
+    borderRadius: theme.shape.borderRadiusMedium,
+    color: theme.palette.text.secondary,
+    ':hover, :focus-visible': {
+        outline: 'none',
+        '.draggable-hover-indicator': {
+            background: theme.palette.table.headerHover,
+        },
+    },
+}));
+
+const DraggableContent = styled('span')(({ theme }) => ({
+    paddingTop: theme.spacing(2),
+    paddingInline: theme.spacing(0.5),
+    display: 'block',
+    height: '100%',
+    width: '100%',
+}));
+
+const DraggableHoverIndicator = styled('span')(({ theme }) => ({
+    display: 'block',
+    paddingBlock: theme.spacing(0.75),
+    borderRadius: theme.shape.borderRadiusMedium,
+    '> svg': {
+        verticalAlign: 'bottom',
+    },
+}));
+
+type Props = {
+    dragItemRef: React.RefObject<HTMLElement>;
+};
+
+export const MilestoneCardDragHandle: FC<Props> = ({ dragItemRef }) => {
+    return (
+        <DragButton tabIndex={-1} type='button'>
+            <DraggableContent ref={dragItemRef}>
+                <DraggableHoverIndicator className='draggable-hover-indicator'>
+                    <DragIndicator aria-hidden />
+                </DraggableHoverIndicator>
+                <ScreenReaderOnly>Drag to reorder</ScreenReaderOnly>
+            </DraggableContent>
+        </DragButton>
+    );
+};
--- a/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneList.tsx
+++ b/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneList.tsx
@ -32,38 +32,44 @@ export const MilestoneList = ({
 }: IMilestoneListProps) => {
    const useNewMilestoneCard = useUiFlag('flagOverviewRedesign');
    const onMoveItem: OnMoveItem = useCallback(
-        async (dragIndex: number, dropIndex: number, save?: boolean) => {
-            if (useNewMilestoneCard && save) {
+        async ({ dragIndex, dropIndex, event, draggedElement }) => {
+            if (useNewMilestoneCard && event.type === 'drop') {
                return; // the user has let go, we should leave the current sort order as it is currently visually displayed
            }

-            if (dragIndex !== dropIndex) {
-                // todo! See if there's a way to make this snippet to stabilize dragging before removing flag `flagOverviewRedesign`
-                // We don't have a reference to `ref` or `event` here, but maybe we can make it work? Somehow?
+            if (event.type === 'dragenter' && dragIndex !== dropIndex) {
+                const target = event.target as HTMLElement;

-                // const { top, bottom } = ref.current.getBoundingClientRect();
-                // const overTargetTop = event.clientY - top < dragItem.height;
-                // const overTargetBottom =
-                //     bottom - event.clientY < dragItem.height;
-                // const draggingUp = dragItem.index > targetIndex;
+                const draggedElementHeight =
+                    draggedElement.getBoundingClientRect().height;

-                // // prevent oscillating by only reordering if there is sufficient space
-                // if (
-                //     (overTargetTop && draggingUp) ||
-                //     (overTargetBottom && !draggingUp)
-                // ) {
-                //     // reorder here
-                // }
-                const oldMilestones = milestones || [];
-                const newMilestones = [...oldMilestones];
-                const movedMilestone = newMilestones.splice(dragIndex, 1)[0];
-                newMilestones.splice(dropIndex, 0, movedMilestone);
+                const { top, bottom } = target.getBoundingClientRect();
+                const overTargetTop =
+                    event.clientY - top < draggedElementHeight;
+                const overTargetBottom =
+                    bottom - event.clientY < draggedElementHeight;
+                const draggingUp = dragIndex > dropIndex;

-                newMilestones.forEach((milestone, index) => {
-                    milestone.sortOrder = index;
-                });
+                // prevent oscillating by only reordering if there is sufficient space
+                const shouldReorder = draggingUp
+                    ? overTargetTop
+                    : overTargetBottom;

-                setMilestones(newMilestones);
+                if (shouldReorder) {
+                    const oldMilestones = milestones || [];
+                    const newMilestones = [...oldMilestones];
+                    const movedMilestone = newMilestones.splice(
+                        dragIndex,
+                        1,
+                    )[0];
+                    newMilestones.splice(dropIndex, 0, movedMilestone);
+
+                    newMilestones.forEach((milestone, index) => {
+                        milestone.sortOrder = index;
+                    });
+
+                    setMilestones(newMilestones);
+                }
            }
        },
        [milestones],
--- a/frontend/src/hooks/useDragItem.ts
+++ b/frontend/src/hooks/useDragItem.ts
@ -1,10 +1,14 @@
 import { useRef, useEffect, type RefObject } from 'react';

-export type OnMoveItem = (
-    dragIndex: number,
-    dropIndex: number,
-    save?: boolean,
-) => void;
+type OnMoveItemParams = {
+    dragIndex: number;
+    dropIndex: number;
+    save: boolean;
+    event: DragEvent;
+    draggedElement: HTMLElement;
+};
+
+export type OnMoveItem = (args: OnMoveItemParams) => void;

 // The element being dragged in the browser.
 let globalDraggedElement: HTMLElement | null;
@ -37,11 +41,17 @@ const addEventListeners = (
 ): (() => void) => {
    const handleEl = handle ?? el;

-    const moveDraggedElement = (save: boolean) => {
+    const moveDraggedElement = (save: boolean, event: DragEvent) => {
        if (globalDraggedElement) {
-            const fromIndex = Number(globalDraggedElement.dataset.index);
-            const toIndex = Number(el.dataset.index);
-            onMoveItem(fromIndex, toIndex, save);
+            const dragIndex = Number(globalDraggedElement.dataset.index);
+            const dropIndex = Number(el.dataset.index);
+            onMoveItem({
+                dragIndex,
+                dropIndex,
+                save,
+                event,
+                draggedElement: globalDraggedElement,
+            });
        }
    };

@ -60,16 +70,16 @@ const addEventListeners = (
        globalDraggedElement = el;
    };

-    const onDragEnter = () => {
-        moveDraggedElement(false);
+    const onDragEnter = (event: DragEvent) => {
+        moveDraggedElement(false, event);
    };

    const onDragOver = (event: DragEvent) => {
        event.preventDefault();
    };

-    const onDrop = () => {
-        moveDraggedElement(true);
+    const onDrop = (event: DragEvent) => {
+        moveDraggedElement(true, event);
        globalDraggedElement = null;
    };