-
Notifications
You must be signed in to change notification settings - Fork 28
Use hook for drag and drop #5062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c2edab1
ca1c44c
f1fc1bc
f880dbe
49c617a
c799873
84d9662
6a5c2bb
7ce4da3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1730,11 +1730,10 @@ describe('App', () => { | |
| ]) | ||
| }) | ||
|
|
||
| it('should show a drop target at the end of the custom plots when moving a plot inside the section but not over any other plot', () => { | ||
| it('should show a drop target at the end of the custom plots when moving a plot inside the section', () => { | ||
| renderAppWithOptionalDataInDragAndDropMode( | ||
| { | ||
| custom: customPlotsFixture, | ||
| template: templatePlotsFixture | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Q] From the test title it sounds like these plots are needed. Do the statements need to be updated?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was never testing the other part. I guess we need to update the statement. |
||
| custom: customPlotsFixture | ||
| }, | ||
| true | ||
| ) | ||
|
|
@@ -1746,11 +1745,10 @@ describe('App', () => { | |
| expect(screen.getByTestId('plot_drop-target')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('should show a drop target a plot at the end of the custom plots when moving a plot inside the section but not over any other plot', () => { | ||
| it('should show a drop target a plot at the end of the custom plots when moving a plot inside the section', () => { | ||
| renderAppWithOptionalDataInDragAndDropMode( | ||
| { | ||
| custom: customPlotsFixture, | ||
| template: templatePlotsFixture | ||
| custom: customPlotsFixture | ||
| }, | ||
| true | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,98 +1,66 @@ | ||
| import { PlotsSection } from 'dvc/src/plots/webview/contract' | ||
| import React from 'react' | ||
| import { useDispatch } from 'react-redux' | ||
| import cx from 'classnames' | ||
| import { DropTarget } from './DropTarget' | ||
| import { changeDragAndDropMode } from './util' | ||
| import styles from './styles.module.scss' | ||
| import { DragAndDropPlot } from './DragAndDropPlot' | ||
| import { plotDataStore } from './plotDataStore' | ||
| import { VirtualizedGrid } from '../../shared/components/virtualizedGrid/VirtualizedGrid' | ||
| import { | ||
| DragDropContainer, | ||
| OnDrop, | ||
| WrapperProps | ||
| } from '../../shared/components/dragDrop/DragDropContainer' | ||
| import { withScale } from '../../util/styles' | ||
| import { OnDrop } from '../../shared/hooks/useDragAndDrop' | ||
|
|
||
| interface DragAndDropGridProps { | ||
| order: string[] | ||
| setOrder: (order: string[]) => void | ||
| groupId: string | ||
| onDrop?: OnDrop | ||
| nbItemsPerRow: number | ||
| useVirtualizedGrid?: boolean | ||
| parentDraggedOver?: boolean | ||
| multiView?: boolean | ||
| sectionKey: PlotsSection | ||
| onDrop?: OnDrop | ||
| } | ||
|
|
||
| export const DragAndDropGrid: React.FC<DragAndDropGridProps> = ({ | ||
| order, | ||
| setOrder, | ||
| groupId, | ||
| onDrop, | ||
| nbItemsPerRow, | ||
| useVirtualizedGrid, | ||
| parentDraggedOver, | ||
| multiView, | ||
| sectionKey | ||
| sectionKey, | ||
| onDrop | ||
| }) => { | ||
| const dispatch = useDispatch() | ||
| const plotClassName = cx(styles.plot, styles.dragAndDropPlot, { | ||
| [styles.multiViewPlot]: multiView | ||
| }) | ||
| const items = order.map((plot: string) => { | ||
| const items = order.map((plot: string, i: number) => { | ||
| const colSpan = | ||
| (multiView && | ||
| plotDataStore[PlotsSection.TEMPLATE_PLOTS][plot].revisions?.length) || | ||
| plotDataStore[PlotsSection.TEMPLATE_PLOTS][plot]?.revisions?.length) || | ||
| 1 | ||
|
|
||
| return ( | ||
| <div | ||
| <DragAndDropPlot | ||
| key={plot} | ||
| id={plot} | ||
| className={plotClassName} | ||
| data-testid={`plot_${plot}`} | ||
| style={withScale(colSpan)} | ||
| > | ||
| <DragAndDropPlot plot={plot} sectionKey={sectionKey} /> | ||
| </div> | ||
| plot={plot} | ||
| sectionKey={sectionKey} | ||
| className={plotClassName} | ||
| colSpan={colSpan} | ||
| group={groupId} | ||
| isParentDraggedOver={parentDraggedOver} | ||
| setOrder={setOrder} | ||
| order={order} | ||
| isLast={i === order.length - 1} | ||
| afterOnDrop={onDrop} | ||
| /> | ||
| ) | ||
| }) | ||
|
|
||
| const handleOnDrop = ( | ||
| draggedId: string, | ||
| draggedGroup: string, | ||
| groupId: string, | ||
| position: number | ||
| ) => { | ||
| changeDragAndDropMode(sectionKey, dispatch, true) | ||
| onDrop?.(draggedId, draggedGroup, groupId, position) | ||
| } | ||
|
|
||
| const handleDragEnd = () => { | ||
| changeDragAndDropMode(sectionKey, dispatch, true) | ||
| } | ||
|
|
||
| return ( | ||
| <DragDropContainer | ||
| order={order} | ||
| setOrder={setOrder} | ||
| items={items} | ||
| group={groupId} | ||
| onDrop={handleOnDrop} | ||
| dropTarget={<DropTarget />} | ||
| wrapperComponent={ | ||
| useVirtualizedGrid | ||
| ? { | ||
| component: VirtualizedGrid as React.FC<WrapperProps>, | ||
| props: { nbItemsPerRow } | ||
| } | ||
| : undefined | ||
| } | ||
| parentDraggedOver={parentDraggedOver} | ||
| onDragEnd={handleDragEnd} | ||
| /> | ||
| return useVirtualizedGrid ? ( | ||
| <VirtualizedGrid nbItemsPerRow={nbItemsPerRow}>{items}</VirtualizedGrid> | ||
| ) : ( | ||
| <>{items}</> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,62 +1,121 @@ | ||
| import cx from 'classnames' | ||
| import React, { useEffect, useRef } from 'react' | ||
| import React, { | ||
| DragEvent, | ||
| HTMLAttributes, | ||
| createRef, | ||
| useEffect, | ||
| useRef | ||
| } from 'react' | ||
| import { useDispatch } from 'react-redux' | ||
| import { PlotsSection } from 'dvc/src/plots/webview/contract' | ||
| import styles from './styles.module.scss' | ||
| import { changeDragAndDropMode } from './util' | ||
| import { DropTarget } from './DropTarget' | ||
| import { GripIcon } from '../../shared/components/dragDrop/GripIcon' | ||
| import { Icon } from '../../shared/components/Icon' | ||
| import { GraphLine } from '../../shared/components/icons' | ||
| import { withScale } from '../../util/styles' | ||
| import { OnDrop, useDragAndDrop } from '../../shared/hooks/useDragAndDrop' | ||
| import { useGetTitles } from '../hooks/useGetTitles' | ||
| import { DragDropItemWithTarget } from '../../shared/components/dragDrop/DragDropItemWithTarget' | ||
|
|
||
| interface DragAndDropPlotProps { | ||
| interface DragAndDropPlotProps extends HTMLAttributes<HTMLDivElement> { | ||
| plot: string | ||
| group: string | ||
| sectionKey: PlotsSection | ||
| colSpan: number | ||
| isParentDraggedOver?: boolean | ||
| setOrder: (order: string[]) => void | ||
| order: string[] | ||
| isLast?: boolean | ||
| afterOnDrop?: OnDrop | ||
| } | ||
|
|
||
| export const DragAndDropPlot: React.FC<DragAndDropPlotProps> = ({ | ||
| plot, | ||
| sectionKey | ||
| sectionKey, | ||
| colSpan, | ||
| group, | ||
| isParentDraggedOver, | ||
| setOrder, | ||
| order, | ||
| isLast, | ||
| afterOnDrop, | ||
| ...props | ||
| }) => { | ||
| const dispatch = useDispatch() | ||
| const dragAndDropTimeout = useRef(0) | ||
| const ref = createRef<HTMLDivElement>() | ||
| const titles = useGetTitles(sectionKey, plot) | ||
| const title = titles?.title || '' | ||
| const subtitle = titles?.subtitle || '' | ||
|
|
||
| const handleDragEnd = () => changeDragAndDropMode(sectionKey, dispatch, true) | ||
|
|
||
| const { isAfter, target, onDragStart, ...dragAndDropProps } = useDragAndDrop({ | ||
| dropTarget: <DropTarget />, | ||
| group, | ||
| id: plot, | ||
| isLast, | ||
| isParentDraggedOver, | ||
| onDragEnd: handleDragEnd, | ||
| onDrop: afterOnDrop, | ||
| order, | ||
| setOrder, | ||
| style: withScale(colSpan) | ||
| }) | ||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| clearTimeout(dragAndDropTimeout.current) | ||
| } | ||
| }, []) | ||
|
|
||
| const handleDragStart = (e: DragEvent<HTMLElement>) => { | ||
| onDragStart(e) | ||
| // Because the dragged element is being created while being dragged for plots in grids, there is a problem where | ||
| // the dragend event is not being associated with the element. Re-adding the event makes sure it's being called. | ||
| ref.current?.addEventListener('dragend', dragAndDropProps.onDragEnd) | ||
| } | ||
|
|
||
| const handleEndOfDragAndDrop = () => { | ||
| // This makes sure every onDrop and onDragEnd events have been called before switching to normal mode | ||
| dragAndDropTimeout.current = window.setTimeout(() => { | ||
| changeDragAndDropMode(sectionKey, dispatch, true) | ||
| handleDragEnd() | ||
| }, 100) | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| { | ||
| // eslint-disable-next-line jsx-a11y/no-static-element-interactions | ||
| <div onMouseUp={handleEndOfDragAndDrop}> | ||
| <GripIcon className={styles.plotGripIcon} /> | ||
| <DragDropItemWithTarget | ||
| isAfter={isAfter} | ||
| dropTarget={target || null} | ||
| draggable={<div />} | ||
| > | ||
| <div | ||
| {...props} | ||
| {...dragAndDropProps} | ||
| onDragStart={handleDragStart} | ||
| ref={ref} | ||
| > | ||
| { | ||
| // eslint-disable-next-line jsx-a11y/no-static-element-interactions | ||
| <div onMouseUp={handleEndOfDragAndDrop}> | ||
| <GripIcon className={styles.plotGripIcon} /> | ||
| </div> | ||
| } | ||
| <div className={styles.dragAndDropPlotContent}> | ||
| <h2 className={styles.dragAndDropPlotTitle}>{title}</h2> | ||
| {subtitle && ( | ||
| <h3 className={styles.dragAndDropPlotSubtitle}>{subtitle}</h3> | ||
| )} | ||
| <Icon | ||
| icon={GraphLine} | ||
| className={cx(styles.dropIcon, styles.onPlotDropIcon)} | ||
| width={70} | ||
| height={70} | ||
| /> | ||
| </div> | ||
| } | ||
| <div className={styles.dragAndDropPlotContent}> | ||
| <h2 className={styles.dragAndDropPlotTitle}>{title}</h2> | ||
| {subtitle && ( | ||
| <h3 className={styles.dragAndDropPlotSubtitle}>{subtitle}</h3> | ||
| )} | ||
| <Icon | ||
| icon={GraphLine} | ||
| className={cx(styles.dropIcon, styles.onPlotDropIcon)} | ||
| width={70} | ||
| height={70} | ||
| /> | ||
| </div> | ||
| </> | ||
| </DragDropItemWithTarget> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -535,14 +535,9 @@ describe('ComparisonTable', () => { | |
|
|
||
| const [headerWrapper] = getHeaders() | ||
|
|
||
| // eslint-disable-next-line testing-library/no-node-access | ||
| expect(headerWrapper.childElementCount).toBe(2) | ||
| expect( | ||
| // eslint-disable-next-line testing-library/no-node-access | ||
| Object.values(headerWrapper.children) | ||
| .map(child => child.id) | ||
| .includes(endingNode.id) | ||
| ).toBe(true) | ||
| within(headerWrapper).getByTestId('comparison-drop-target') | ||
| ).toBeInTheDocument() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Q] How do these assertions differ/relate to one another?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not wrapped the exact same way, but the test checks the same thing, it was just not written in a clear way before. |
||
| }) | ||
|
|
||
| it('should not change the order when dropping a header in its own spot', () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for adding this? I'm not seeing any new files with this syntax 🤔
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useDragAndDrop.tsxthere is somejsxin that file