Make drop zones larger when dragging in the same section (#2206)

* Make drop zone larger for when dragging in template plots

* Fix tests

* Add larger drop zones to checkpoint plots

* Reduce number of lines

* Reduce lines even more for DragDropContainer

* Reduce lines even more for DragDropContainer

* Add tests

* Apply review comment

Co-authored-by: mattseddon <[email protected]>

Author: sroy3

webview/src/plots/components/App.test.tsx

@@ -801,13 +801,75 @@ describe('App', () => {
 
     dragEnter(
       anotherSingleViewPlot,
-      'plots-section_template-single_0',
-      DragEnterDirection.RIGHT
+      'template-single_0',
+      DragEnterDirection.LEFT
     )
 
     expect(screen.getByTestId('plot_drop-target')).toBeInTheDocument()
   })
 
+  it('should show a drop target at the end of the template plots section when moving a plot inside of one section but not over any other plot', () => {
+    renderAppWithOptionalData({
+      template: complexTemplatePlotsFixture
+    })
+
+    const aSingleViewPlot = screen.getByTestId(join('plot_other', 'plot.tsv'))
+
+    dragEnter(aSingleViewPlot, 'template-single_0', DragEnterDirection.LEFT)
+
+    expect(screen.getByTestId('plot_drop-target')).toBeInTheDocument()
+  })
+
+  it('should drop a plot at the end of the template plots section when moving a plot inside of one section but not over any other plot', () => {
+    renderAppWithOptionalData({
+      template: complexTemplatePlotsFixture
+    })
+
+    const aSingleViewPlot = screen.getByTestId(join('plot_other', 'plot.tsv'))
+    const topSection = screen.getByTestId('plots-section_template-single_0')
+
+    dragAndDrop(aSingleViewPlot, topSection)
+    const plots = within(topSection).getAllByTestId(/^plot_/)
+
+    expect(plots.map(plot => plot.id)).toStrictEqual([
+      join('logs', 'loss.tsv'),
+      join('other', 'plot.tsv')
+    ])
+  })
+
+  it('should show a drop target at the end of the checkpoint plots when moving a plot inside the section but not over any other plot', () => {
+    renderAppWithOptionalData({
+      checkpoint: checkpointPlotsFixture
+    })
+
+    const plots = screen.getAllByTestId(/summary\.json/)
+
+    dragEnter(plots[0], 'checkpoint-plots', DragEnterDirection.LEFT)
+
+    expect(screen.getByTestId('plot_drop-target')).toBeInTheDocument()
+  })
+
+  it('should show a drop a plot at the end of the checkpoint plots when moving a plot inside the section but not over any other plot', () => {
+    renderAppWithOptionalData({
+      checkpoint: checkpointPlotsFixture
+    })
+
+    const plots = screen.getAllByTestId(/summary\.json/)
+
+    dragAndDrop(plots[0], screen.getByTestId('checkpoint-plots'))
+
+    const expectedOrder = [
+      'summary.json:accuracy',
+      'summary.json:val_loss',
+      'summary.json:val_accuracy',
+      'summary.json:loss'
+    ]
+
+    expect(
+      screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
+    ).toStrictEqual(expectedOrder)
+  })
+
   it('should show a drop zone when hovering a new section', () => {
     renderAppWithOptionalData({
       template: complexTemplatePlotsFixture

webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx

@@ -1,4 +1,4 @@
-import React, { useEffect, useState } from 'react'
+import React, { DragEvent, useEffect, useState } from 'react'
 import { useSelector } from 'react-redux'
 import cx from 'classnames'
 import { ColorScale } from 'dvc/src/plots/webview/contract'
@@ -17,6 +17,7 @@ import { VirtualizedGrid } from '../../../shared/components/virtualizedGrid/Virt
 import { shouldUseVirtualizedGrid } from '../util'
 import { useNbItemsPerRow } from '../../hooks/useNbItemsPerRow'
 import { PlotsState } from '../../store'
+import { changeOrderWithDraggedInfo } from '../../../util/array'
 
 interface CheckpointPlotsProps {
   plotsIds: string[]
@@ -29,7 +30,11 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({
 }) => {
   const [order, setOrder] = useState(plotsIds)
   const { size, hasData } = useSelector((state: PlotsState) => state.checkpoint)
+  const [onSection, setOnSection] = useState(false)
   const nbItemsPerRow = useNbItemsPerRow(size)
+  const draggedRef = useSelector(
+    (state: PlotsState) => state.dragAndDrop.draggedRef
+  )
 
   useEffect(() => {
     setOrder(pastOrder => performSimpleOrderedUpdate(pastOrder, plotsIds))
@@ -55,11 +60,26 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({
 
   const useVirtualizedGrid = shouldUseVirtualizedGrid(items.length, size)
 
+  const handleDropAtTheEnd = () => {
+    setMetricOrder(changeOrderWithDraggedInfo(order, draggedRef))
+  }
+
+  const handleDragOver = (e: DragEvent) => {
+    e.preventDefault()
+    setOnSection(true)
+  }
+
   return items.length > 0 ? (
     <div
+      data-testid="checkpoint-plots"
+      id="checkpoint-plots"
       className={cx(styles.singleViewPlotsGrid, {
         [styles.noBigGrid]: !useVirtualizedGrid
       })}
+      onDragEnter={() => setOnSection(true)}
+      onDragLeave={() => setOnSection(false)}
+      onDragOver={handleDragOver}
+      onDrop={handleDropAtTheEnd}
     >
       <DragDropContainer
         order={order}
@@ -76,6 +96,7 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({
               }
             : undefined
         }
+        parentDraggedOver={onSection}
       />
     </div>
   ) : (

webview/src/plots/components/templatePlots/TemplatePlots.tsx

@@ -7,6 +7,7 @@ import React, { DragEvent, useState, useEffect, useRef } from 'react'
 import cx from 'classnames'
 import { useDispatch, useSelector } from 'react-redux'
 import { MessageFromWebviewType } from 'dvc/src/webview/contract'
+import { reorderObjectList } from 'dvc/src/util/array'
 import { AddedSection } from './AddedSection'
 import { TemplatePlotsGrid } from './TemplatePlotsGrid'
 import { removeFromPreviousAndAddToNewSection } from './util'
@@ -19,6 +20,8 @@ import { PlotsState } from '../../store'
 import { plotDataStore } from '../plotDataStore'
 import { setDraggedOverGroup } from '../../../shared/components/dragDrop/dragDropSlice'
 import { EmptyState } from '../../../shared/components/emptyState/EmptyState'
+import { isSameGroup } from '../../../shared/components/dragDrop/DragDropContainer'
+import { changeOrderWithDraggedInfo } from '../../../util/array'
 
 export enum NewSectionBlock {
   TOP = 'drop-section-top',
@@ -120,7 +123,7 @@ export const TemplatePlots: React.FC = () => {
     draggedId: string,
     draggedGroup: string,
     groupId: string,
-    position: number
+    position?: number
   ) => {
     if (draggedGroup === groupId) {
       return
@@ -169,34 +172,69 @@ export const TemplatePlots: React.FC = () => {
 
         const isMultiView = section.group === TemplatePlotGroup.MULTI_VIEW
 
-        const classes = cx({
+        const classes = cx(styles.sectionWrapper, {
           [styles.multiViewPlotsGrid]: isMultiView,
           [styles.singleViewPlotsGrid]: !isMultiView,
           [styles.noBigGrid]: !useVirtualizedGrid
         })
 
+        const handleDropAtTheEnd = () => {
+          handleEnteringSection('')
+          if (!draggedRef) {
+            return
+          }
+
+          if (draggedRef.group === groupId) {
+            const order = section.entries.map(s => s.id)
+            const updatedSections = [...sections]
+
+            const newOrder = changeOrderWithDraggedInfo(order, draggedRef)
+            updatedSections[i] = {
+              ...sections[i],
+              entries: reorderObjectList<TemplatePlotEntry>(
+                newOrder,
+                section.entries,
+                'id'
+              )
+            }
+            setSections(updatedSections)
+          } else if (isSameGroup(draggedRef.group, groupId)) {
+            handleDropInSection(
+              draggedRef.itemId,
+              draggedRef.group,
+              groupId,
+              section.entries.length
+            )
+          }
+        }
+
+        const handleDragOver = (e: DragEvent) => {
+          e.preventDefault()
+          handleEnteringSection(groupId)
+        }
+
         return (
-          section.entries.length > 0 && (
-            <div
-              key={groupId}
-              id={groupId}
-              data-testid={`plots-section_${groupId}`}
-              className={classes}
-              onDragEnter={() => handleEnteringSection(groupId)}
-            >
-              <TemplatePlotsGrid
-                entries={section.entries}
-                groupId={groupId}
-                groupIndex={i}
-                onDropInSection={handleDropInSection}
-                multiView={isMultiView}
-                setSectionEntries={setSectionEntries}
-                useVirtualizedGrid={useVirtualizedGrid}
-                nbItemsPerRow={nbItemsPerRow}
-                parentDraggedOver={draggedOverGroup === groupId}
-              />
-            </div>
-          )
+          <div
+            key={groupId}
+            id={groupId}
+            data-testid={`plots-section_${groupId}`}
+            className={classes}
+            onDragEnter={() => handleEnteringSection(groupId)}
+            onDragOver={handleDragOver}
+            onDrop={handleDropAtTheEnd}
+          >
+            <TemplatePlotsGrid
+              entries={section.entries}
+              groupId={groupId}
+              groupIndex={i}
+              onDropInSection={handleDropInSection}
+              multiView={isMultiView}
+              setSectionEntries={setSectionEntries}
+              useVirtualizedGrid={useVirtualizedGrid}
+              nbItemsPerRow={nbItemsPerRow}
+              parentDraggedOver={draggedOverGroup === groupId}
+            />
+          </div>
         )
       })}
       <AddedSection

webview/src/plots/components/templatePlots/util.ts

@@ -5,12 +5,10 @@ import {
 
 const remove = (section: TemplatePlotSection, entryId: string) => {
   const entries = section.entries.filter(({ id }) => id !== entryId)
-  return entries.length > 0
-    ? {
-        entries,
-        group: section.group
-      }
-    : null
+  return {
+    entries,
+    group: section.group
+  }
 }
 
 const add = (
@@ -27,21 +25,27 @@ const add = (
   return { entries, group: section.group }
 }
 
+const cleanup = (section: TemplatePlotSection) =>
+  section.entries.length > 0 ? section : null
+
 export const removeFromPreviousAndAddToNewSection = (
   sections: TemplatePlotSection[],
   oldSectionIndex: number,
   entryId: string,
   newGroupIndex?: number,
   entry?: TemplatePlotEntry,
   position?: number
-) =>
-  sections
-    .map((section, i) => {
-      if (i === oldSectionIndex) {
-        return remove(section, entryId)
-      } else if (i === newGroupIndex && entry) {
-        return add(section, entry, position)
-      }
-      return section
-    })
+) => {
+  const newSections = sections.map((section, i) => {
+    if (i === oldSectionIndex) {
+      return remove(section, entryId)
+    } else if (i === newGroupIndex && entry) {
+      return add(section, entry, position)
+    }
+    return section
+  })
+
+  return newSections
+    .map(section => cleanup(section))
     .filter(Boolean) as TemplatePlotSection[]
+}