From 92f86d1b6f516a1e1d337eae8d84c9c82821ed3a Mon Sep 17 00:00:00 2001 From: Stephanie Roy Date: Thu, 18 Aug 2022 11:19:44 -0400 Subject: [PATCH] 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 <37993418+mattseddon@users.noreply.github.com> --- webview/src/plots/components/App.test.tsx | 66 ++++++++++++++- .../checkpointPlots/CheckpointPlots.tsx | 23 ++++- .../templatePlots/TemplatePlots.tsx | 84 ++++++++++++++----- .../plots/components/templatePlots/util.ts | 36 ++++---- .../components/dragDrop/DragDropContainer.tsx | 84 ++++++++++++------- .../shared/components/dragDrop/DropTarget.tsx | 19 +---- webview/src/util/array.ts | 16 ++++ 7 files changed, 241 insertions(+), 87 deletions(-) diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 8157ad06c6..433071bd3e 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/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 diff --git a/webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx b/webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx index ce5e9bbc9d..ec461f19d8 100644 --- a/webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx +++ b/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 = ({ }) => { 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 = ({ const useVirtualizedGrid = shouldUseVirtualizedGrid(items.length, size) + const handleDropAtTheEnd = () => { + setMetricOrder(changeOrderWithDraggedInfo(order, draggedRef)) + } + + const handleDragOver = (e: DragEvent) => { + e.preventDefault() + setOnSection(true) + } + return items.length > 0 ? (
setOnSection(true)} + onDragLeave={() => setOnSection(false)} + onDragOver={handleDragOver} + onDrop={handleDropAtTheEnd} > = ({ } : undefined } + parentDraggedOver={onSection} />
) : ( diff --git a/webview/src/plots/components/templatePlots/TemplatePlots.tsx b/webview/src/plots/components/templatePlots/TemplatePlots.tsx index 10d5368868..4bece7b245 100644 --- a/webview/src/plots/components/templatePlots/TemplatePlots.tsx +++ b/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( + 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 && ( -
handleEnteringSection(groupId)} - > - -
- ) +
handleEnteringSection(groupId)} + onDragOver={handleDragOver} + onDrop={handleDropAtTheEnd} + > + +
) })} { 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,6 +25,9 @@ const add = ( return { entries, group: section.group } } +const cleanup = (section: TemplatePlotSection) => + section.entries.length > 0 ? section : null + export const removeFromPreviousAndAddToNewSection = ( sections: TemplatePlotSection[], oldSectionIndex: number, @@ -34,14 +35,17 @@ export const removeFromPreviousAndAddToNewSection = ( 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[] +} diff --git a/webview/src/shared/components/dragDrop/DragDropContainer.tsx b/webview/src/shared/components/dragDrop/DragDropContainer.tsx index c170460c45..bebb7f0f19 100644 --- a/webview/src/shared/components/dragDrop/DragDropContainer.tsx +++ b/webview/src/shared/components/dragDrop/DragDropContainer.tsx @@ -25,7 +25,7 @@ const orderIdxTune = (direction: DragEnterDirection, isAfter: boolean) => { return isAfter ? -1 : 0 } -const isSameGroup = (group1?: string, group2?: string) => +export const isSameGroup = (group1?: string, group2?: string) => getIDWithoutIndex(group1) === getIDWithoutIndex(group2) export type WrapperProps = { @@ -72,33 +72,44 @@ export const DragDropContainer: React.FC = ({ // eslint-disable-next-line sonarjs/cognitive-complexity }) => { const [draggedOverId, setDraggedOverId] = useState('') + const [hoveringSomething, setHoveringSomething] = useState(false) + const isHovering = useRef(false) const [draggedId, setDraggedId] = useState('') const [direction, setDirection] = useState(DragEnterDirection.LEFT) const { draggedRef, draggedOverGroup } = useSelector( (state: PlotsState) => state.dragAndDrop ) const draggedOverIdTimeout = useRef(0) + const hoveringTimeout = useRef(0) const dispatch = useDispatch() const cleanup = () => { + setHoveringSomething(false) + isHovering.current = false setDraggedOverId('') setDraggedId('') setDirection(DragEnterDirection.LEFT) } useEffect(() => { - return () => clearTimeout(draggedOverIdTimeout.current) + return () => { + clearTimeout(draggedOverIdTimeout.current) + clearTimeout(hoveringTimeout.current) + } }, []) useEffect(() => { cleanup() }, [order]) + if (items.length === 0) { + return null + } + const createGhostStyle = (e: DragEvent) => { if (ghostElemStyle) { for (const [rule, value] of Object.entries(ghostElemStyle)) { - const prop = getStyleProperty(rule) - e.currentTarget.style[prop] = value + e.currentTarget.style[getStyleProperty(rule)] = value } } } @@ -107,8 +118,7 @@ export const DragDropContainer: React.FC = ({ const elem = idToNode(id) if (elem && ghostElemStyle) { for (const [rule] of Object.entries(ghostElemStyle)) { - const prop = getStyleProperty(rule) - elem.style[prop] = '' + elem.style[getStyleProperty(rule)] = '' } } } @@ -124,7 +134,6 @@ export const DragDropContainer: React.FC = ({ toIdx = 0 } } - const itemIndex = idx.toString() e.dataTransfer.effectAllowed = 'move' e.dataTransfer.dropEffect = 'move' @@ -134,7 +143,7 @@ export const DragDropContainer: React.FC = ({ changeRef({ group, itemId: id, - itemIndex + itemIndex: idx.toString() }) ) setDraggedId(id) @@ -147,10 +156,13 @@ export const DragDropContainer: React.FC = ({ droppedIndex: number, draggedIndex: number, newOrder: string[], - oldDraggedId: string + oldDraggedId: string, + isNew: boolean ) => { + if (isNew && draggedRef) { + newOrder.push(draggedRef.itemId) + } const dragged = newOrder[draggedIndex] - newOrder.splice(draggedIndex, 1) newOrder.splice(droppedIndex, 0, dragged) @@ -161,38 +173,37 @@ export const DragDropContainer: React.FC = ({ } const handleOnDrop = (e: DragEvent) => { + e.stopPropagation() if (!draggedRef) { return } - const dragged = draggedRef.itemId - draggedOverIdTimeout.current = window.setTimeout(() => { setDraggedId('') }, 0) + const dragged = draggedRef.itemId if (dragged === draggedOverId) { dispatch(changeRef(undefined)) return } - const newOrder = [...order] const isNew = !order.includes(dragged) - if (isNew) { - newOrder.push(dragged) - } - const draggedIndex = isNew - ? newOrder.length - 1 - : getIDIndex(draggedRef.itemIndex) - + const draggedIndex = isNew ? order.length : getIDIndex(draggedRef.itemIndex) const droppedIndex = order.indexOf(e.currentTarget.id.split('__')[0]) const orderIdxChange = orderIdxTune(direction, droppedIndex > draggedIndex) const orderIdxChanged = droppedIndex + orderIdxChange const isEnabled = !disabledDropIds.includes(order[orderIdxChanged]) if (isEnabled && isSameGroup(draggedRef.group, group)) { - applyDrop(orderIdxChanged, draggedIndex, newOrder, dragged) + applyDrop(orderIdxChanged, draggedIndex, [...order], dragged, isNew) } } + const setIsHovering = () => { + setHoveringSomething(true) + isHovering.current = true + } + const handleDragEnter = (e: DragEvent) => { + setIsHovering() if (isSameGroup(draggedRef?.group, group)) { const { id } = e.currentTarget if ( @@ -209,6 +220,9 @@ export const DragDropContainer: React.FC = ({ const handleDragOver = (e: DragEvent) => { e.preventDefault() + if (draggedOverId && e.currentTarget.id.includes(draggedOverId)) { + setIsHovering() + } if (isSameGroup(draggedRef?.group, group)) { const { id } = e.currentTarget !disabledDropIds.includes(id) && @@ -222,6 +236,15 @@ export const DragDropContainer: React.FC = ({ cleanup() } + const handleDragLeave = () => { + isHovering.current = false + hoveringTimeout.current = window.setTimeout(() => { + if (!isHovering.current) { + setHoveringSomething(false) + } + }, 500) + } + const buildItem = (id: string, draggable: JSX.Element) => ( = ({ onDragOver={handleDragOver} onDragEnter={handleDragEnter} onDrop={handleOnDrop} + onDragLeave={handleDragLeave} draggable={!disabledDropIds.includes(id)} style={ (!shouldShowOnDrag && id === draggedId && { display: 'none' }) || @@ -251,6 +275,7 @@ export const DragDropContainer: React.FC = ({ = ({ const createItemWithDropTarget = (id: string, item: JSX.Element) => { const isEnteringRight = direction === DragEnterDirection.RIGHT - const target = + const isSameGroup = draggedOverGroup === group || draggedRef?.group === group - ? getTarget(id, isEnteringRight) - : null + const target = isSameGroup ? getTarget(id, isEnteringRight) : null const itemWithTag = shouldShowOnDrag ? (
@@ -288,17 +312,17 @@ export const DragDropContainer: React.FC = ({ const { id } = draggable.props const item = id && buildItem(id, draggable) - return id === draggedOverId ? createItemWithDropTarget(id, item) : item + return id === draggedOverId && (hoveringSomething || !parentDraggedOver) + ? createItemWithDropTarget(id, item) + : item }) if ( isSameGroup(draggedRef?.group, group) && - draggedRef?.itemId !== draggedId && - !draggedOverId && - parentDraggedOver && - wrappedItems.length > 0 + !hoveringSomething && + parentDraggedOver ) { - const lastId = wrappedItems[wrappedItems.length - 1].id + const lastId = wrappedItems[wrappedItems.length - 1].props.id wrappedItems.push(getTarget(lastId, false)) } diff --git a/webview/src/shared/components/dragDrop/DropTarget.tsx b/webview/src/shared/components/dragDrop/DropTarget.tsx index 4c2158e152..cefe4972f2 100644 --- a/webview/src/shared/components/dragDrop/DropTarget.tsx +++ b/webview/src/shared/components/dragDrop/DropTarget.tsx @@ -1,27 +1,16 @@ -import React, { DragEventHandler } from 'react' +import React, { HTMLAttributes } from 'react' -interface DropTargetProps { +interface DropTargetProps extends HTMLAttributes { children: JSX.Element - onDragOver: DragEventHandler - onDrop: DragEventHandler id: string - className?: string } export const DropTarget: React.FC = ({ children, - onDragOver, - onDrop, id, - className + ...props }) => ( -
+
{children}
) diff --git a/webview/src/util/array.ts b/webview/src/util/array.ts index bbc0572976..9c78439f87 100644 --- a/webview/src/util/array.ts +++ b/webview/src/util/array.ts @@ -1,5 +1,6 @@ import isEqual from 'lodash.isequal' import { BaseType } from './objects' +import { DraggedInfo } from '../shared/components/dragDrop/dragDropSlice' export const pushIf = (array: T[], condition: boolean, elements: T[]) => condition && array.push(...elements) @@ -9,3 +10,18 @@ export const keepEqualOldReferencesInArray = ( newArray: BaseType[] ) => newArray.map(item => oldArray.find(oldItem => isEqual(oldItem, item)) || item) + +export const changeOrderWithDraggedInfo = ( + order: string[], + dragged: DraggedInfo +) => { + if (!dragged) { + return order + } + const newOrder = [...order] + const draggedIndex = Number.parseInt(dragged.itemIndex, 10) + + newOrder.splice(draggedIndex, 1) + newOrder.push(dragged.itemId) + return newOrder +}