From b48c80d19aa0990bb16c33acd3c607a897b418a0 Mon Sep 17 00:00:00 2001 From: Stephanie Roy Date: Wed, 8 Mar 2023 11:44:59 -0500 Subject: [PATCH] Select the number of items per row from slider (#3405) * Added a story to visualize the icons * Disable chromatic snapshots * Select the number of items per row from picker * Use const values * Use slider instead * Resize multiview plots * Self review * Fix and add tests * Apply review comments --- extension/src/plots/model/index.test.ts | 20 +-- extension/src/plots/model/index.ts | 16 +- extension/src/plots/vega/util.test.ts | 34 ++-- extension/src/plots/vega/util.ts | 13 +- extension/src/plots/webview/contract.ts | 18 +-- .../fixtures/expShow/base/checkpointPlots.ts | 4 +- .../test/fixtures/expShow/base/customPlots.ts | 4 +- .../src/test/fixtures/plotsDiff/index.ts | 14 +- .../src/test/suite/experiments/index.test.ts | 4 +- extension/src/test/suite/plots/index.test.ts | 10 +- webview/src/plots/components/App.test.tsx | 152 +++++++++--------- webview/src/plots/components/Plots.tsx | 8 +- .../src/plots/components/PlotsContainer.tsx | 51 +++++- webview/src/plots/components/Resizer.tsx | 139 ---------------- webview/src/plots/components/ZoomablePlot.tsx | 73 +-------- .../CheckpointPlotsWrapper.tsx | 20 ++- .../customPlots/CustomPlotsWrapper.tsx | 9 +- .../src/plots/components/styles.module.scss | 65 +------- .../templatePlots/TemplatePlotsWrapper.tsx | 6 + webview/src/plots/components/util.ts | 13 +- webview/src/plots/components/webviewSlice.ts | 22 +-- .../shared/components/slider/MinMaxSlider.tsx | 42 +++++ .../components/slider/styles.module.scss | 17 ++ .../virtualizedGrid/VirtualizedGrid.tsx | 4 +- .../src/stories/ComparisonTable.stories.tsx | 6 +- webview/src/stories/Plots.stories.tsx | 27 ++-- webview/src/stories/Slider.stories.tsx | 32 ++++ .../src/test/mouseEventsWithCoordinates.ts | 31 ---- 28 files changed, 328 insertions(+), 526 deletions(-) delete mode 100644 webview/src/plots/components/Resizer.tsx create mode 100644 webview/src/shared/components/slider/MinMaxSlider.tsx create mode 100644 webview/src/shared/components/slider/styles.module.scss create mode 100644 webview/src/stories/Slider.stories.tsx delete mode 100644 webview/src/test/mouseEventsWithCoordinates.ts diff --git a/extension/src/plots/model/index.test.ts b/extension/src/plots/model/index.test.ts index 20671e03cc..fb93079713 100644 --- a/extension/src/plots/model/index.test.ts +++ b/extension/src/plots/model/index.test.ts @@ -1,8 +1,8 @@ import { PlotsModel } from '.' import { + DEFAULT_NB_ITEMS_PER_REOW, DEFAULT_SECTION_COLLAPSED, DEFAULT_SECTION_NB_ITEMS_PER_ROW, - PlotNumberOfItemsPerRow, Section } from '../webview/contract' import { buildMockMemento } from '../../test/util' @@ -69,33 +69,25 @@ describe('plotsModel', () => { it('should change the plotSize when calling setPlotSize', () => { expect(model.getNbItemsPerRow(Section.CHECKPOINT_PLOTS)).toStrictEqual( - PlotNumberOfItemsPerRow.TWO + DEFAULT_NB_ITEMS_PER_REOW ) - model.setNbItemsPerRow( - Section.CHECKPOINT_PLOTS, - PlotNumberOfItemsPerRow.ONE - ) + model.setNbItemsPerRow(Section.CHECKPOINT_PLOTS, 1) - expect(model.getNbItemsPerRow(Section.CHECKPOINT_PLOTS)).toStrictEqual( - PlotNumberOfItemsPerRow.ONE - ) + expect(model.getNbItemsPerRow(Section.CHECKPOINT_PLOTS)).toStrictEqual(1) }) it('should update the persisted plot size when calling setPlotSize', () => { const mementoUpdateSpy = jest.spyOn(memento, 'update') - model.setNbItemsPerRow( - Section.CHECKPOINT_PLOTS, - PlotNumberOfItemsPerRow.TWO - ) + model.setNbItemsPerRow(Section.CHECKPOINT_PLOTS, 2) expect(mementoUpdateSpy).toHaveBeenCalledTimes(1) expect(mementoUpdateSpy).toHaveBeenCalledWith( PersistenceKey.PLOT_NB_ITEMS_PER_ROW + exampleDvcRoot, { ...DEFAULT_SECTION_NB_ITEMS_PER_ROW, - [Section.CHECKPOINT_PLOTS]: PlotNumberOfItemsPerRow.TWO + [Section.CHECKPOINT_PLOTS]: 2 } ) }) diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index ebc3044cb7..1ccec4fada 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -27,8 +27,8 @@ import { Section, SectionCollapsed, CustomPlotData, - PlotNumberOfItemsPerRow, - DEFAULT_HEIGHT + DEFAULT_HEIGHT, + DEFAULT_NB_ITEMS_PER_REOW } from '../webview/contract' import { ExperimentsOutput, @@ -408,18 +408,10 @@ export class PlotsModel extends ModelWithPersistence { } public getNbItemsPerRow(section: Section) { - if ( - this.nbItemsPerRow[section] && - [ - PlotNumberOfItemsPerRow.ONE, - PlotNumberOfItemsPerRow.TWO, - PlotNumberOfItemsPerRow.THREE, - PlotNumberOfItemsPerRow.FOUR - ].includes(this.nbItemsPerRow[section]) - ) { + if (this.nbItemsPerRow[section]) { return this.nbItemsPerRow[section] } - return PlotNumberOfItemsPerRow.TWO + return DEFAULT_NB_ITEMS_PER_REOW } public setHeight(section: Section, height: number | undefined) { diff --git a/extension/src/plots/vega/util.test.ts b/extension/src/plots/vega/util.test.ts index 6d449aebd8..3be7f092e0 100644 --- a/extension/src/plots/vega/util.test.ts +++ b/extension/src/plots/vega/util.test.ts @@ -18,8 +18,8 @@ import scatterTemplate from '../../test/fixtures/plotsDiff/templates/scatter' import smoothTemplate from '../../test/fixtures/plotsDiff/templates/smooth' import multiSourceTemplate from '../../test/fixtures/plotsDiff/templates/multiSource' import { copyOriginalColors } from '../../experiments/model/status/colors' -import { PlotNumberOfItemsPerRow } from '../webview/contract' import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract' +import { DEFAULT_NB_ITEMS_PER_REOW } from '../webview/contract' describe('isMultiViewPlot', () => { it('should recognize the confusion matrix template as a multi view plot', () => { @@ -86,7 +86,7 @@ describe('extendVegaSpec', () => { it('should not add encoding if no color scale is provided', () => { const extendedSpec = extendVegaSpec( linearTemplate, - PlotNumberOfItemsPerRow.TWO + DEFAULT_NB_ITEMS_PER_REOW ) expect(extendedSpec.encoding).toBeUndefined() }) @@ -98,7 +98,7 @@ describe('extendVegaSpec', () => { } const extendedSpec = extendVegaSpec( linearTemplate, - PlotNumberOfItemsPerRow.TWO, + DEFAULT_NB_ITEMS_PER_REOW, { color: colorScale } @@ -143,7 +143,7 @@ describe('extendVegaSpec', () => { it('should truncate all titles from the left to 50 characters for large plots', () => { const spec = withLongTemplatePlotTitle() - const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.ONE) + const updatedSpec = extendVegaSpec(spec, 1) const truncatedTitle = '…-many-many-characters-at-least-seventy-characters' const truncatedHorizontalTitle = @@ -169,7 +169,7 @@ describe('extendVegaSpec', () => { it('should truncate all titles from the left to 50 characters for regular plots', () => { const spec = withLongTemplatePlotTitle() - const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.TWO) + const updatedSpec = extendVegaSpec(spec, DEFAULT_NB_ITEMS_PER_REOW) const truncatedTitle = '…-many-many-characters-at-least-seventy-characters' const truncatedHorizontalTitle = @@ -195,7 +195,7 @@ describe('extendVegaSpec', () => { it('should truncate all titles from the left to 30 characters for small plots', () => { const spec = withLongTemplatePlotTitle() - const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.THREE) + const updatedSpec = extendVegaSpec(spec, 3) const truncatedTitle = '…s-at-least-seventy-characters' const truncatedHorizontalTitle = '…at-least-seventy-characters-x' @@ -225,7 +225,7 @@ describe('extendVegaSpec', () => { text: repeatedTitle }) - const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.THREE) + const updatedSpec = extendVegaSpec(spec, 3) const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890' @@ -242,7 +242,7 @@ describe('extendVegaSpec', () => { const repeatedTitle = 'abcdefghijklmnopqrstuvwyz1234567890' const spec = withLongTemplatePlotTitle([repeatedTitle, repeatedTitle]) - const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.THREE) + const updatedSpec = extendVegaSpec(spec, 3) const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890' @@ -262,7 +262,7 @@ describe('extendVegaSpec', () => { text: [repeatedTitle, repeatedTitle] }) - const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.THREE) + const updatedSpec = extendVegaSpec(spec, 3) const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890' @@ -276,17 +276,13 @@ describe('extendVegaSpec', () => { }) it('should update the multi-source template to remove erroneous shape encoding from the vertical line displayed on hover', () => { - const updatedSpec = extendVegaSpec( - multiSourceTemplate, - PlotNumberOfItemsPerRow.ONE, - { - color: { domain: [], range: [] }, - shape: { - field: 'field', - scale: { domain: [], range: [] } - } + const updatedSpec = extendVegaSpec(multiSourceTemplate, 1, { + color: { domain: [], range: [] }, + shape: { + field: 'field', + scale: { domain: [], range: [] } } - ) + }) expect(updatedSpec.encoding).not.toBeUndefined() expect(updatedSpec.layer[1].layer[0].encoding.shape).toBeNull() diff --git a/extension/src/plots/vega/util.ts b/extension/src/plots/vega/util.ts index 4654795904..c1c8ecdb05 100644 --- a/extension/src/plots/vega/util.ts +++ b/extension/src/plots/vega/util.ts @@ -26,7 +26,7 @@ import { } from 'vega-lite/build/src/spec/repeat' import { TopLevelUnitSpec } from 'vega-lite/build/src/spec/unit' import isEqual from 'lodash.isequal' -import { ColorScale, PlotNumberOfItemsPerRow } from '../webview/contract' +import { ColorScale, DEFAULT_NB_ITEMS_PER_REOW } from '../webview/contract' import { ShapeEncoding, StrokeDashEncoding } from '../multiSource/constants' import { Color } from '../../experiments/model/status/colors' @@ -220,13 +220,6 @@ const truncateTitleAsArrayOrString = (title: Text, size: number) => { return truncateTitleString(title as unknown as string, size) } -const TitleLimit = { - [PlotNumberOfItemsPerRow.ONE]: 50, - [PlotNumberOfItemsPerRow.TWO]: 50, - [PlotNumberOfItemsPerRow.THREE]: 30, - [PlotNumberOfItemsPerRow.FOUR]: 30 -} - const truncateTitlePart = ( title: Title, key: 'text' | 'subtitle', @@ -261,7 +254,7 @@ const truncateTitle = ( } export const truncateVerticalTitle = (title: Text | Title, size: number) => - truncateTitle(title, TitleLimit[size] * 0.75) + truncateTitle(title, (size > 2 ? 30 : 50) * 0.75) const isEndValue = (valueType: string) => ['string', 'number', 'boolean'].includes(valueType) @@ -289,7 +282,7 @@ export const truncateTitles = ( const title = value as unknown as Title specCopy[key] = vertical ? truncateVerticalTitle(title, size) - : truncateTitle(title, TitleLimit[size]) + : truncateTitle(title, size > DEFAULT_NB_ITEMS_PER_REOW ? 30 : 50) } else if (isEndValue(valueType)) { specCopy[key] = value } else if (Array.isArray(value)) { diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts index 2bc8149aca..eca1fdb3ec 100644 --- a/extension/src/plots/webview/contract.ts +++ b/extension/src/plots/webview/contract.ts @@ -1,15 +1,7 @@ import { VisualizationSpec } from 'react-vega' import { Color } from '../../experiments/model/status/colors' -// It is easier to keep the numerical order than the alphabetical one -/* eslint-disable sort-keys-fix/sort-keys-fix */ -export const PlotNumberOfItemsPerRow = { - ONE: 1, - TWO: 2, - THREE: 3, - FOUR: 4 -} -/* eslint-enable sort-keys-fix/sort-keys-fix */ +export const DEFAULT_NB_ITEMS_PER_REOW = 2 export enum Section { CHECKPOINT_PLOTS = 'checkpoint-plots', @@ -19,10 +11,10 @@ export enum Section { } export const DEFAULT_SECTION_NB_ITEMS_PER_ROW = { - [Section.CHECKPOINT_PLOTS]: PlotNumberOfItemsPerRow.TWO, - [Section.TEMPLATE_PLOTS]: PlotNumberOfItemsPerRow.TWO, - [Section.COMPARISON_TABLE]: PlotNumberOfItemsPerRow.TWO, - [Section.CUSTOM_PLOTS]: PlotNumberOfItemsPerRow.TWO + [Section.CHECKPOINT_PLOTS]: DEFAULT_NB_ITEMS_PER_REOW, + [Section.TEMPLATE_PLOTS]: DEFAULT_NB_ITEMS_PER_REOW, + [Section.COMPARISON_TABLE]: DEFAULT_NB_ITEMS_PER_REOW, + [Section.CUSTOM_PLOTS]: DEFAULT_NB_ITEMS_PER_REOW } // Height is undefined by default because it is calculated by ratio of the width it'll fill (calculated by the webview) diff --git a/extension/src/test/fixtures/expShow/base/checkpointPlots.ts b/extension/src/test/fixtures/expShow/base/checkpointPlots.ts index 4b4f620e02..6e09b5baa8 100644 --- a/extension/src/test/fixtures/expShow/base/checkpointPlots.ts +++ b/extension/src/test/fixtures/expShow/base/checkpointPlots.ts @@ -1,7 +1,7 @@ import { copyOriginalColors } from '../../../../experiments/model/status/colors' import { CheckpointPlotsData, - PlotNumberOfItemsPerRow + DEFAULT_NB_ITEMS_PER_REOW } from '../../../../plots/webview/contract' const colors = copyOriginalColors() @@ -91,7 +91,7 @@ const data: CheckpointPlotsData = { 'summary.json:val_loss', 'summary.json:val_accuracy' ], - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW, height: undefined } diff --git a/extension/src/test/fixtures/expShow/base/customPlots.ts b/extension/src/test/fixtures/expShow/base/customPlots.ts index 67bcfa8d62..34700a4958 100644 --- a/extension/src/test/fixtures/expShow/base/customPlots.ts +++ b/extension/src/test/fixtures/expShow/base/customPlots.ts @@ -1,6 +1,6 @@ import { CustomPlotsData, - PlotNumberOfItemsPerRow + DEFAULT_NB_ITEMS_PER_REOW } from '../../../../plots/webview/contract' const data: CustomPlotsData = { @@ -50,7 +50,7 @@ const data: CustomPlotsData = { ] } ], - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW, height: undefined } diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index 2e21c954fe..31c3e083b5 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -11,9 +11,9 @@ import { TemplatePlotGroup, TemplatePlotsData, TemplatePlots, - PlotNumberOfItemsPerRow, Revision, - PlotsComparisonData + PlotsComparisonData, + DEFAULT_NB_ITEMS_PER_REOW } from '../../../plots/webview/contract' import { join } from '../../util/path' import { copyOriginalColors } from '../../../experiments/model/status/colors' @@ -499,7 +499,7 @@ const extendedSpecs = (plotsOutput: TemplatePlots): TemplatePlotSection[] => { ) || [] } } as TopLevelSpec, - PlotNumberOfItemsPerRow.TWO, + DEFAULT_NB_ITEMS_PER_REOW, { color: { domain: expectedRevisions, @@ -659,14 +659,14 @@ export const getRevisions = (): Revision[] => { export const getMinimalWebviewMessage = () => ({ plots: extendedSpecs(basicVega), - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW, height: undefined, revisions: getRevisions() }) export const getTemplateWebviewMessage = (): TemplatePlotsData => ({ plots: extendedSpecs({ ...basicVega, ...require('./vega').default }), - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW, height: undefined }) @@ -676,7 +676,7 @@ export const getManyTemplatePlotsWebviewMessage = ( plots: extendedSpecs({ ...multipleVega(length) }), - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW, height: undefined }) @@ -703,7 +703,7 @@ export const getComparisonWebviewMessage = ( return { revisions: getRevisions(), plots: plotAcc, - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW, height: undefined } } diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index 19478e4925..4b20c2cd7c 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -67,7 +67,6 @@ import { DvcExecutor } from '../../../cli/dvc/executor' import { shortenForLabel } from '../../../util/string' import { GitExecutor } from '../../../cli/git/executor' import { WorkspacePlots } from '../../../plots/workspace' -import { PlotNumberOfItemsPerRow } from '../../../plots/webview/contract' import { RegisteredCliCommands, RegisteredCommands @@ -82,6 +81,7 @@ import * as ProcessExecution from '../../../process/execution' import { DvcReader } from '../../../cli/dvc/reader' import { Connect } from '../../../connect' import { DvcViewer } from '../../../cli/dvc/viewer' +import { DEFAULT_NB_ITEMS_PER_REOW } from '../../../plots/webview/contract' const { openFileInEditor } = FileSystem @@ -339,7 +339,7 @@ suite('Experiments Test Suite', () => { ).returns(undefined) const mockColumnId = 'params:params.yaml:lr' - const mockWidth = PlotNumberOfItemsPerRow.TWO + const mockWidth = DEFAULT_NB_ITEMS_PER_REOW mockMessageReceived.fire({ payload: { id: mockColumnId, width: mockWidth }, diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 8c4e45683d..fd38a1db90 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -25,7 +25,6 @@ import { dvcDemoPath } from '../../util' import { DEFAULT_SECTION_COLLAPSED, PlotsData as TPlotsData, - PlotNumberOfItemsPerRow, Section, TemplatePlotGroup, TemplatePlotsData @@ -248,23 +247,20 @@ suite('Plots Test Suite', () => { mockMessageReceived.fire({ payload: { height: undefined, - nbItemsPerRow: PlotNumberOfItemsPerRow.THREE, + nbItemsPerRow: 3, section: Section.TEMPLATE_PLOTS }, type: MessageFromWebviewType.RESIZE_PLOTS }) expect(mockSetPlotSize).to.be.calledOnce - expect(mockSetPlotSize).to.be.calledWithExactly( - Section.TEMPLATE_PLOTS, - PlotNumberOfItemsPerRow.THREE - ) + expect(mockSetPlotSize).to.be.calledWithExactly(Section.TEMPLATE_PLOTS, 3) expect(mockSendTelemetryEvent).to.be.calledOnce expect(mockSendTelemetryEvent).to.be.calledWithExactly( EventName.VIEWS_PLOTS_SECTION_RESIZED, { height: undefined, - nbItemsPerRow: PlotNumberOfItemsPerRow.THREE, + nbItemsPerRow: 3, section: Section.TEMPLATE_PLOTS }, undefined diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 8e917e8580..7408c24cfd 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -23,12 +23,12 @@ import { CheckpointPlotsData, DEFAULT_SECTION_COLLAPSED, PlotsData, - PlotNumberOfItemsPerRow, PlotsType, Revision, Section, TemplatePlotGroup, - TemplatePlotsData + TemplatePlotsData, + DEFAULT_NB_ITEMS_PER_REOW } from 'dvc/src/plots/webview/contract' import { MessageFromWebviewType, @@ -46,7 +46,7 @@ import { plotDataStore, TemplatePlotsById } from './plotDataStore' -import { setSnapPoints } from './webviewSlice' +import { setMaxNbPlotsPerRow } from './webviewSlice' import { plotsReducers, plotsStore } from '../store' import { vsCodeApi } from '../../shared/api' import { @@ -59,7 +59,6 @@ import { DragEnterDirection } from '../../shared/components/dragDrop/util' import { clearSelection, createWindowTextSelection } from '../../test/selection' import * as EventCurrentTargetDistances from '../../shared/components/dragDrop/currentTarget' import { OVERSCAN_ROW_COUNT } from '../../shared/components/virtualizedGrid/VirtualizedGrid' -import { pickAndMove } from '../../test/mouseEventsWithCoordinates' jest.mock('../../shared/components/dragDrop/currentTarget', () => { const actualModule = jest.requireActual( @@ -139,9 +138,9 @@ describe('App', () => { return store } - const setWrapperSize = (store: typeof plotsStore) => + const setWrapperSize = (store: typeof plotsStore, size = 2000) => act(() => { - store.dispatch(setSnapPoints(1000)) + store.dispatch(setMaxNbPlotsPerRow(size)) }) const templatePlot = templatePlotsFixture.plots[0].entries[0] @@ -264,7 +263,7 @@ describe('App', () => { checkpoint: null, comparison: { height: undefined, - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW, plots: [ { path: 'training/plots/images/misclassified.jpg', @@ -697,68 +696,61 @@ describe('App', () => { }) }) - it('should send a message to the extension with the selected size when changing the size of plots', () => { + it('should display a slider to pick the number of items per row if there are items and the action is available', () => { const store = renderAppWithOptionalData({ checkpoint: checkpointPlotsFixture }) + setWrapperSize(store) - const plotResizer = screen.getAllByTestId('vertical-plot-resizer')[0] + expect(screen.getByTestId('nb-items-per-row-slider')).toBeInTheDocument() + }) + it('should not display a slider to pick the number of items per row if there are no items', () => { + const store = renderAppWithOptionalData({}) setWrapperSize(store) - pickAndMove(plotResizer, 10) - expect(mockPostMessage).toHaveBeenCalledWith({ - payload: { - nbItemsPerRow: PlotNumberOfItemsPerRow.ONE, - section: Section.CHECKPOINT_PLOTS - }, - type: MessageFromWebviewType.RESIZE_PLOTS - }) - setWrapperSize(store) - pickAndMove(plotResizer, -10) - expect(mockPostMessage).toHaveBeenCalledWith({ - payload: { - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO, - section: Section.CHECKPOINT_PLOTS - }, - type: MessageFromWebviewType.RESIZE_PLOTS - }) + expect( + screen.queryByTestId('nb-items-per-row-slider') + ).not.toBeInTheDocument() + }) + it('should not display a slider to pick the number of items per row if the action is unavailable', () => { + const store = renderAppWithOptionalData({ + comparison: comparisonTableFixture + }) setWrapperSize(store) - pickAndMove(plotResizer, -10) + expect( + screen.queryByTestId('nb-items-per-row-slider') + ).not.toBeInTheDocument() + }) - expect(mockPostMessage).toHaveBeenCalledWith({ - payload: { - nbItemsPerRow: PlotNumberOfItemsPerRow.THREE, - section: Section.CHECKPOINT_PLOTS - }, - type: MessageFromWebviewType.RESIZE_PLOTS + it('should not display a slider to pick the number of items per row if the only width available for one item per row or less', () => { + const store = renderAppWithOptionalData({ + checkpoint: checkpointPlotsFixture }) + setWrapperSize(store, 400) - setWrapperSize(store) - pickAndMove(plotResizer, -10) - expect(mockPostMessage).toHaveBeenCalledWith({ - payload: { - nbItemsPerRow: PlotNumberOfItemsPerRow.FOUR, - section: Section.CHECKPOINT_PLOTS - }, - type: MessageFromWebviewType.RESIZE_PLOTS - }) + expect( + screen.queryByTestId('nb-items-per-row-slider') + ).not.toBeInTheDocument() }) - it('should not send a message to the extension with the selected size when changing the size of plots and pressing escape', () => { + it('should send a message to the extension with the selected size when changing the size of plots', () => { const store = renderAppWithOptionalData({ checkpoint: checkpointPlotsFixture }) + setWrapperSize(store) - const plotResizer = screen.getAllByTestId('vertical-plot-resizer')[0] + const plotResizer = within( + screen.getByTestId('nb-items-per-row-slider') + ).getByRole('slider') - setWrapperSize(store) - pickAndMove(plotResizer, 10, 0, true) - expect(mockPostMessage).not.toHaveBeenCalledWith({ + fireEvent.change(plotResizer, { target: { value: -3 } }) + expect(mockPostMessage).toHaveBeenCalledWith({ payload: { - nbItemsPerRow: PlotNumberOfItemsPerRow.ONE, + height: undefined, + nbItemsPerRow: 3, section: Section.CHECKPOINT_PLOTS }, type: MessageFromWebviewType.RESIZE_PLOTS @@ -1540,7 +1532,7 @@ describe('App', () => { const resizeScreen = (width: number, store: typeof plotsStore) => { act(() => { - store.dispatch(setSnapPoints(width)) + store.dispatch(setMaxNbPlotsPerRow(width)) }) act(() => { global.innerWidth = width @@ -1549,10 +1541,10 @@ describe('App', () => { } describe('Large plots', () => { - it('should wrap the checkpoint plots in a big grid (virtualize them) when there are more than ten large plots', async () => { + it('should wrap the checkpoint plots in a big grid (virtualize them) when there are more than eight large plots', async () => { await renderAppAndChangeSize( - { checkpoint: createCheckpointPlots(11) }, - PlotNumberOfItemsPerRow.ONE, + { checkpoint: createCheckpointPlots(9) }, + 1, Section.CHECKPOINT_PLOTS ) @@ -1567,10 +1559,10 @@ describe('App', () => { expect(screen.getByRole('grid')).toBeInTheDocument() }) - it('should not wrap the checkpoint plots in a big grid (virtualize them) when there are ten or fewer large plots', async () => { + it('should not wrap the checkpoint plots in a big grid (virtualize them) when there are eight or fewer large plots', async () => { await renderAppAndChangeSize( - { checkpoint: createCheckpointPlots(10) }, - PlotNumberOfItemsPerRow.ONE, + { checkpoint: createCheckpointPlots(8) }, + 1, Section.CHECKPOINT_PLOTS ) @@ -1585,10 +1577,10 @@ describe('App', () => { expect(screen.queryByRole('grid')).not.toBeInTheDocument() }) - it('should wrap the template plots in a big grid (virtualize them) when there are more than ten large plots', async () => { + it('should wrap the template plots in a big grid (virtualize them) when there are more than eight large plots', async () => { await renderAppAndChangeSize( - { template: manyTemplatePlots(11) }, - PlotNumberOfItemsPerRow.ONE, + { template: manyTemplatePlots(9) }, + 1, Section.TEMPLATE_PLOTS ) @@ -1603,10 +1595,10 @@ describe('App', () => { expect(screen.getByRole('grid')).toBeInTheDocument() }) - it('should not wrap the template plots in a big grid (virtualize them) when there are ten or fewer large plots', async () => { + it('should not wrap the template plots in a big grid (virtualize them) when there are eight or fewer large plots', async () => { await renderAppAndChangeSize( - { template: manyTemplatePlots(10) }, - PlotNumberOfItemsPerRow.ONE, + { template: manyTemplatePlots(8) }, + 1, Section.TEMPLATE_PLOTS ) @@ -1628,7 +1620,7 @@ describe('App', () => { beforeEach(async () => { store = await renderAppAndChangeSize( { checkpoint }, - PlotNumberOfItemsPerRow.ONE, + 1, Section.CHECKPOINT_PLOTS ) }) @@ -1678,40 +1670,40 @@ describe('App', () => { }) describe('Regular plots', () => { - it('should wrap the checkpoint plots in a big grid (virtualize them) when there are more than fifteen regular plots', async () => { + it('should wrap the checkpoint plots in a big grid (virtualize them) when there are more than fourteen regular plots', async () => { await renderAppAndChangeSize( - { checkpoint: createCheckpointPlots(16) }, - PlotNumberOfItemsPerRow.TWO, + { checkpoint: createCheckpointPlots(15) }, + DEFAULT_NB_ITEMS_PER_REOW, Section.CHECKPOINT_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() }) - it('should not wrap the checkpoint plots in a big grid (virtualize them) when there are eight or fifteen regular plots', async () => { + it('should not wrap the checkpoint plots in a big grid (virtualize them) when there are fourteen regular plots', async () => { await renderAppAndChangeSize( - { checkpoint: createCheckpointPlots(15) }, - PlotNumberOfItemsPerRow.TWO, + { checkpoint: createCheckpointPlots(14) }, + DEFAULT_NB_ITEMS_PER_REOW, Section.CHECKPOINT_PLOTS ) expect(screen.queryByRole('grid')).not.toBeInTheDocument() }) - it('should wrap the template plots in a big grid (virtualize them) when there are more than fifteen regular plots', async () => { + it('should wrap the template plots in a big grid (virtualize them) when there are more than fourteen regular plots', async () => { await renderAppAndChangeSize( - { template: manyTemplatePlots(16) }, - PlotNumberOfItemsPerRow.TWO, + { template: manyTemplatePlots(15) }, + DEFAULT_NB_ITEMS_PER_REOW, Section.TEMPLATE_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() }) - it('should not wrap the template plots in a big grid (virtualize them) when there are fifteen or fewer regular plots', async () => { + it('should not wrap the template plots in a big grid (virtualize them) when there are fourteen or fewer regular plots', async () => { await renderAppAndChangeSize( - { template: manyTemplatePlots(15) }, - PlotNumberOfItemsPerRow.TWO, + { template: manyTemplatePlots(14) }, + DEFAULT_NB_ITEMS_PER_REOW, Section.TEMPLATE_PLOTS ) @@ -1725,7 +1717,7 @@ describe('App', () => { beforeEach(async () => { store = await renderAppAndChangeSize( { checkpoint }, - PlotNumberOfItemsPerRow.TWO, + DEFAULT_NB_ITEMS_PER_REOW, Section.CHECKPOINT_PLOTS ) }) @@ -1778,7 +1770,7 @@ describe('App', () => { it('should wrap the checkpoint plots in a big grid (virtualize them) when there are more than twenty small plots', async () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(21) }, - PlotNumberOfItemsPerRow.FOUR, + 4, Section.CHECKPOINT_PLOTS ) @@ -1788,7 +1780,7 @@ describe('App', () => { it('should not wrap the checkpoint plots in a big grid (virtualize them) when there are twenty or fewer small plots', async () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(20) }, - PlotNumberOfItemsPerRow.FOUR, + 4, Section.CHECKPOINT_PLOTS ) @@ -1798,7 +1790,7 @@ describe('App', () => { it('should wrap the template plots in a big grid (virtualize them) when there are more than twenty small plots', async () => { await renderAppAndChangeSize( { template: manyTemplatePlots(21) }, - PlotNumberOfItemsPerRow.FOUR, + 4, Section.TEMPLATE_PLOTS ) @@ -1808,7 +1800,7 @@ describe('App', () => { it('should not wrap the template plots in a big grid (virtualize them) when there are twenty or fewer small plots', async () => { await renderAppAndChangeSize( { template: manyTemplatePlots(20) }, - PlotNumberOfItemsPerRow.FOUR, + 4, Section.TEMPLATE_PLOTS ) @@ -1822,7 +1814,7 @@ describe('App', () => { beforeEach(async () => { store = await renderAppAndChangeSize( { checkpoint }, - PlotNumberOfItemsPerRow.FOUR, + 4, Section.CHECKPOINT_PLOTS ) }) diff --git a/webview/src/plots/components/Plots.tsx b/webview/src/plots/components/Plots.tsx index 57db03b854..8d893258dd 100644 --- a/webview/src/plots/components/Plots.tsx +++ b/webview/src/plots/components/Plots.tsx @@ -7,14 +7,13 @@ import { CustomPlotsWrapper } from './customPlots/CustomPlotsWrapper' import { TemplatePlotsWrapper } from './templatePlots/TemplatePlotsWrapper' import { ComparisonTableWrapper } from './comparisonTable/ComparisonTableWrapper' import { Ribbon } from './ribbon/Ribbon' -import { setSnapPoints, setZoomedInPlot } from './webviewSlice' +import { setMaxNbPlotsPerRow, setZoomedInPlot } from './webviewSlice' import { EmptyState } from '../../shared/components/emptyState/EmptyState' import { Modal } from '../../shared/components/modal/Modal' import { WebviewWrapper } from '../../shared/components/webviewWrapper/WebviewWrapper' import { GetStarted } from '../../shared/components/getStarted/GetStarted' import { PlotsState } from '../store' -// eslint-disable-next-line sonarjs/cognitive-complexity const PlotsContent = () => { const dispatch = useDispatch() const { hasData, hasPlots, hasUnselectedPlots, zoomedInPlot } = useSelector( @@ -35,7 +34,10 @@ const PlotsContent = () => { const onResize = () => { wrapperRef.current && dispatch( - setSnapPoints(wrapperRef.current.getBoundingClientRect().width - 100) + setMaxNbPlotsPerRow( + // Plots grid have a 20px margin around it, we subtract 20 * 2 from the wrapper width to get the max available space + wrapperRef.current.getBoundingClientRect().width - 40 + ) ) } window.addEventListener('resize', onResize) diff --git a/webview/src/plots/components/PlotsContainer.tsx b/webview/src/plots/components/PlotsContainer.tsx index 6c775a725a..4af70c8553 100644 --- a/webview/src/plots/components/PlotsContainer.tsx +++ b/webview/src/plots/components/PlotsContainer.tsx @@ -3,8 +3,11 @@ import React, { MouseEvent, useEffect, DetailedHTMLProps, - HTMLAttributes + HTMLAttributes, + useCallback } from 'react' +import { AnyAction } from '@reduxjs/toolkit' +import { useDispatch, useSelector } from 'react-redux' import { Section } from 'dvc/src/plots/webview/contract' import { MessageFromWebviewType } from 'dvc/src/webview/contract' import { PlotsPicker, PlotsPickerProps } from './PlotsPicker' @@ -24,16 +27,20 @@ import { } from '../../shared/components/icons' import { isSelecting } from '../../util/strings' import { isTooltip } from '../../util/helpers' +import { MinMaxSlider } from '../../shared/components/slider/MinMaxSlider' +import { PlotsState } from '../store' export interface PlotsContainerProps { sectionCollapsed: boolean sectionKey: Section title: string nbItemsPerRow: number + changeNbItemsPerRow?: (nb: number) => AnyAction menu?: PlotsPickerProps addPlotsButton?: { onClick: () => void } removePlotsButton?: { onClick: () => void } children: React.ReactNode + hasItems?: boolean } export const SectionDescription = { @@ -96,9 +103,15 @@ export const PlotsContainer: React.FC = ({ nbItemsPerRow, menu, addPlotsButton, - removePlotsButton + removePlotsButton, + changeNbItemsPerRow, + hasItems }) => { const open = !sectionCollapsed + const dispatch = useDispatch() + const maxNbPlotsPerRow = useSelector( + (state: PlotsState) => state.webview.maxNbPlotsPerRow + ) useEffect(() => { window.dispatchEvent(new Event('resize')) @@ -152,6 +165,24 @@ export const PlotsContainer: React.FC = ({ } } + const handleResize = useCallback( + (nbItems: number) => { + if (changeNbItemsPerRow) { + const positiveNbItems = Math.abs(nbItems) + dispatch(changeNbItemsPerRow(positiveNbItems)) + sendMessage({ + payload: { + height: undefined, + nbItemsPerRow: positiveNbItems, + section: sectionKey + }, + type: MessageFromWebviewType.RESIZE_PLOTS + }) + } + }, + [dispatch, changeNbItemsPerRow, sectionKey] + ) + return (
@@ -173,11 +204,25 @@ export const PlotsContainer: React.FC = ({
+ {changeNbItemsPerRow && hasItems && maxNbPlotsPerRow > 1 && ( +
+ +
+ )} {open && (
= 4 })} style={ { diff --git a/webview/src/plots/components/Resizer.tsx b/webview/src/plots/components/Resizer.tsx deleted file mode 100644 index 4851e62aee..0000000000 --- a/webview/src/plots/components/Resizer.tsx +++ /dev/null @@ -1,139 +0,0 @@ -import React, { useEffect, useRef, useState } from 'react' -import styles from './styles.module.scss' - -interface ResizerProps { - className: string - onGrab: () => void - onRelease: () => void - onResize: (diff: number) => void - snapPoints: number[] - sizeBetweenResizers: number - setIsExpanding: (isExpanding: boolean) => void - currentSnapPoint: number -} - -export const Resizer: React.FC = ({ - className, - onGrab, - onRelease, - onResize, - snapPoints, - sizeBetweenResizers, // Plot + gap - setIsExpanding, - currentSnapPoint -}) => { - const startingPageX = useRef(0) - const [lockedX, setLockedX] = useState(undefined) - const [isResizing, setIsResizing] = useState(false) - const lockedSnapPoint = useRef(currentSnapPoint) - - // eslint-disable-next-line sonarjs/cognitive-complexity - useEffect(() => { - const setSnapPoint = (snapPoint: number, isShrinking?: boolean) => { - setLockedX( - snapPoint - - sizeBetweenResizers + - 20 * currentSnapPoint * (isShrinking ? -1 : 1) - ) - setIsExpanding(!isShrinking) - lockedSnapPoint.current = snapPoints.indexOf(snapPoint) + 1 - } - const handleMouseMove = (e: MouseEvent) => { - const newDiffX = e.clientX - startingPageX.current - - if (isResizing && newDiffX !== 0) { - const positionX = newDiffX + sizeBetweenResizers - const isShrinking = newDiffX < 0 - if (isShrinking) { - for (let i = currentSnapPoint; i < snapPoints.length; i++) { - const snapPoint = snapPoints[i] - if (Math.abs(positionX) >= snapPoint) { - setSnapPoint(snapPoint, true) - break - } - } - } else { - for (let i = currentSnapPoint - 1; i >= 0; i--) { - const snapPoint = snapPoints[i] - if (positionX <= snapPoint) { - setSnapPoint(snapPoint) - break - } - } - } - } - } - - document.addEventListener('mousemove', handleMouseMove) - - return () => { - document.removeEventListener('mousemove', handleMouseMove) - } - }, [ - currentSnapPoint, - isResizing, - snapPoints, - setIsExpanding, - sizeBetweenResizers - ]) - - useEffect(() => { - const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Escape') { - setIsResizing(false) - setLockedX(undefined) - setIsExpanding(false) - } - } - - document.addEventListener('keydown', handleKeyDown) - - return () => { - document.removeEventListener('keydown', handleKeyDown) - } - }, [setIsResizing, setLockedX, setIsExpanding]) - - useEffect(() => { - const handleMouseUp = () => { - if (isResizing) { - setIsExpanding(false) - onResize(lockedSnapPoint.current) - onRelease() - setLockedX(undefined) - setIsResizing(false) - } - } - - document.addEventListener('mouseup', handleMouseUp) - - return () => { - document.removeEventListener('mouseup', handleMouseUp) - } - }, [onResize, onRelease, isResizing, setIsExpanding]) - - const handleMouseDown = (e: React.MouseEvent) => { - startingPageX.current = e.clientX - setIsResizing(true) - onGrab() - } - - const lockedStyle = lockedX - ? { - right: Math.min(-lockedX, 0), - width: Math.abs(lockedX) - } - : {} - - return ( - // eslint-disable-next-line jsx-a11y/no-static-element-interactions -
- {isResizing && ( -
- )} -
- ) -} diff --git a/webview/src/plots/components/ZoomablePlot.tsx b/webview/src/plots/components/ZoomablePlot.tsx index 231dc43fae..a1d79ad182 100644 --- a/webview/src/plots/components/ZoomablePlot.tsx +++ b/webview/src/plots/components/ZoomablePlot.tsx @@ -1,19 +1,14 @@ import { AnyAction } from '@reduxjs/toolkit' -import cx from 'classnames' -import { MessageFromWebviewType } from 'dvc/src/webview/contract' import { Section } from 'dvc/src/plots/webview/contract' -import React, { useCallback, useEffect, useRef, useState } from 'react' -import { useDispatch, useSelector } from 'react-redux' +import React, { useEffect, useRef } from 'react' +import { useDispatch } from 'react-redux' import { VisualizationSpec } from 'react-vega' import VegaLite, { VegaLiteProps } from 'react-vega/lib/VegaLite' import { setZoomedInPlot } from './webviewSlice' import styles from './styles.module.scss' -import { Resizer } from './Resizer' import { config } from './constants' -import { PlotsState } from '../store' import { useGetPlot } from '../hooks/useGetPlot' import { GripIcon } from '../../shared/components/dragDrop/GripIcon' -import { sendMessage } from '../../shared/vscode' interface ZoomablePlotProps { spec?: VisualizationSpec @@ -30,22 +25,11 @@ export const ZoomablePlot: React.FC = ({ spec: createdSpec, id, onViewReady, - changeDisabledDragIds, - changeSize, - currentSnapPoint, - section, - shouldNotResize + section }) => { - const snapPoints = useSelector( - (state: PlotsState) => state.webview.snapPoints - ) const { data, content: spec } = useGetPlot(section, id, createdSpec) const dispatch = useDispatch() const currentPlotProps = useRef() - const clickDisabled = useRef(false) - const enableClickTimeout = useRef(0) - const [isExpanding, setIsExpanding] = useState(false) - const size = snapPoints[currentSnapPoint - 1] const plotProps: VegaLiteProps = { actions: false, @@ -63,64 +47,17 @@ export const ZoomablePlot: React.FC = ({ ) }, [data, spec, dispatch, id]) - useEffect(() => { - return () => { - window.clearTimeout(enableClickTimeout.current) - } - }, []) - - const handleOnClick = () => - !clickDisabled.current && dispatch(setZoomedInPlot({ id, plot: plotProps })) + const handleOnClick = () => dispatch(setZoomedInPlot({ id, plot: plotProps })) - const onResize = useCallback( - (newSnapPoint: number) => { - dispatch(changeSize(newSnapPoint)) - sendMessage({ - payload: { height: undefined, nbItemsPerRow: newSnapPoint, section }, - type: MessageFromWebviewType.RESIZE_PLOTS - }) - }, - [dispatch, changeSize, section] - ) - - const commonResizerProps = { - onGrab: () => { - clickDisabled.current = true - dispatch(changeDisabledDragIds([id])) - }, - onRelease: () => { - dispatch(changeDisabledDragIds([])) - enableClickTimeout.current = window.setTimeout( - () => (clickDisabled.current = false), - 0 - ) - }, - onResize - } if (!data && !spec) { return null } return ( - ) } diff --git a/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx b/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx index 8c96b8c290..184d2f534a 100644 --- a/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx +++ b/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx @@ -3,6 +3,7 @@ import { MessageFromWebviewType } from 'dvc/src/webview/contract' import React, { useEffect, useState } from 'react' import { useSelector } from 'react-redux' import { CheckpointPlots } from './CheckpointPlots' +import { changeSize } from './checkpointPlotsSlice' import { PlotsContainer } from '../PlotsContainer' import { sendMessage } from '../../../shared/vscode' import { PlotsState } from '../../store' @@ -26,14 +27,15 @@ export const CheckpointPlotsWrapper: React.FC = () => { }) } - const menu = - plotsIds.length > 0 - ? { - plots: metrics, - selectedPlots, - setSelectedPlots: setSelectedMetrics - } - : undefined + const hasItems = plotsIds.length > 0 + + const menu = hasItems + ? { + plots: metrics, + selectedPlots, + setSelectedPlots: setSelectedMetrics + } + : undefined return ( { menu={menu} nbItemsPerRow={nbItemsPerRow} sectionCollapsed={isCollapsed} + changeNbItemsPerRow={changeSize} + hasItems={hasItems} > diff --git a/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx b/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx index 91050032c0..1ffc6e4f22 100644 --- a/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx +++ b/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx @@ -3,6 +3,7 @@ import React, { useEffect, useState } from 'react' import { useSelector } from 'react-redux' import { MessageFromWebviewType } from 'dvc/src/webview/contract' import { CustomPlots } from './CustomPlots' +import { changeSize } from './customPlotsSlice' import { PlotsContainer } from '../PlotsContainer' import { PlotsState } from '../../store' import { sendMessage } from '../../../shared/vscode' @@ -23,6 +24,8 @@ export const CustomPlotsWrapper: React.FC = () => { sendMessage({ type: MessageFromWebviewType.REMOVE_CUSTOM_PLOTS }) } + const hasItems = plotsIds.length > 0 + return ( { nbItemsPerRow={nbItemsPerRow} sectionCollapsed={isCollapsed} addPlotsButton={{ onClick: addCustomPlot }} - removePlotsButton={ - plotsIds.length > 0 ? { onClick: removeCustomPlots } : undefined - } + removePlotsButton={hasItems ? { onClick: removeCustomPlots } : undefined} + changeNbItemsPerRow={changeSize} + hasItems={hasItems} > diff --git a/webview/src/plots/components/styles.module.scss b/webview/src/plots/components/styles.module.scss index d19b4e3231..e279a9610c 100644 --- a/webview/src/plots/components/styles.module.scss +++ b/webview/src/plots/components/styles.module.scss @@ -61,6 +61,7 @@ $gap: 20px; input { accent-color: $accent-color; + outline: none; } } @@ -119,10 +120,6 @@ $gap: 20px; } } -.multiViewPlotsGrid { - padding-right: $gap; -} - .plot { aspect-ratio: 9 / 5; overflow: visible; @@ -172,56 +169,6 @@ $gap: 20px; } } -$resizer-border: 2px dashed $accent-color; -.resizerLocked { - position: absolute; - background-color: $accent-color-transparent; - border: $resizer-border; - border-right: none; - - .plotExpanding & { - border-right: $resizer-border; - border-left: none; - } -} - -.plotExpanding { - &::after { - content: ''; - background-color: $accent-color-transparent; - position: absolute; - top: 0; - left: 0; - width: 100%; - height: 100%; - border: $resizer-border; - border-right: none; - box-sizing: border-box; - } -} - -.plotVerticalResizer { - width: 3px; - height: 100%; - right: 0; - top: 0; - cursor: col-resize; - - &::after { - width: 1px; - height: 50%; - top: 25%; - right: 1px; - } - - .resizerLocked { - width: 3px; - height: 100%; - top: 0; - right: 1px; - } -} - .plot img, .plot svg { max-width: 100%; @@ -232,8 +179,6 @@ $resizer-border: 2px dashed $accent-color; .plot.multiViewPlot { aspect-ratio: calc(0.8 * var(--scale) + 0.2); grid-column: span var(--scale); - width: calc(300px * var(--scale)); - max-width: max-content; } .comparisonTable { @@ -245,7 +190,7 @@ $resizer-border: 2px dashed $accent-color; } .dropSectionWrapper { - height: min-content; + margin-top: -30px; } .dropSection { @@ -343,6 +288,12 @@ $resizer-border: 2px dashed $accent-color; } } +.nbItemsPerRowSlider { + display: flex; + justify-content: end; + margin: 20px; +} + :global(.has-actions) { summary { background: $fg-color !important; diff --git a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx index 82c986df17..69e53024b8 100644 --- a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx +++ b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx @@ -2,6 +2,7 @@ import { Section } from 'dvc/src/plots/webview/contract' import React from 'react' import { useSelector } from 'react-redux' import { TemplatePlots } from './TemplatePlots' +import { changeSize } from './templatePlotsSlice' import { PlotsContainer } from '../PlotsContainer' import { PlotsState } from '../../store' @@ -9,6 +10,9 @@ export const TemplatePlotsWrapper: React.FC = () => { const { nbItemsPerRow, isCollapsed } = useSelector( (state: PlotsState) => state.template ) + const hasItems = useSelector( + (state: PlotsState) => Object.keys(state.template.plotsSnapshots).length > 0 + ) return ( { sectionKey={Section.TEMPLATE_PLOTS} nbItemsPerRow={nbItemsPerRow} sectionCollapsed={isCollapsed} + changeNbItemsPerRow={changeSize} + hasItems={hasItems} > diff --git a/webview/src/plots/components/util.ts b/webview/src/plots/components/util.ts index 157918e267..556daa449d 100644 --- a/webview/src/plots/components/util.ts +++ b/webview/src/plots/components/util.ts @@ -1,15 +1,4 @@ -import { PlotNumberOfItemsPerRow } from 'dvc/src/plots/webview/contract' - -export const DEFAULT_NB_ITEMS_PER_ROW = PlotNumberOfItemsPerRow.TWO - -const MaxItemsBeforeVirtualization = { - [PlotNumberOfItemsPerRow.ONE]: 10, - [PlotNumberOfItemsPerRow.TWO]: 15, - [PlotNumberOfItemsPerRow.THREE]: 18, - [PlotNumberOfItemsPerRow.FOUR]: 20 -} - export const shouldUseVirtualizedGrid = ( nbItems: number, nbItemsPerRow: number -) => nbItems > MaxItemsBeforeVirtualization[nbItemsPerRow] +) => nbItems / nbItemsPerRow > 8 - (nbItemsPerRow - 1) diff --git a/webview/src/plots/components/webviewSlice.ts b/webview/src/plots/components/webviewSlice.ts index c8fe49c611..1f4b803d07 100644 --- a/webview/src/plots/components/webviewSlice.ts +++ b/webview/src/plots/components/webviewSlice.ts @@ -8,23 +8,21 @@ type ZoomedInPlotState = { id: string refresh?: boolean } - -type SnapPoints = [number, number, number, number] export interface WebviewState { hasData: boolean hasPlots: boolean hasUnselectedPlots: boolean selectedRevisions: Revision[] zoomedInPlot: ZoomedInPlotState | undefined - snapPoints: SnapPoints + maxNbPlotsPerRow: number } export const webviewInitialState: WebviewState = { hasData: false, hasPlots: false, hasUnselectedPlots: false, + maxNbPlotsPerRow: 4, selectedRevisions: [], - snapPoints: [0, 0, 0, 0], zoomedInPlot: { id: '', plot: undefined @@ -38,17 +36,13 @@ export const webviewSlice = createSlice({ initialize: (state: { hasData: boolean }) => { state.hasData = true }, - setSnapPoints: ( - state: { snapPoints: SnapPoints }, + setMaxNbPlotsPerRow: ( + state: { maxNbPlotsPerRow: number }, action: PayloadAction ) => { - // Action payload here is the max width of the plots webview. When changed, we re-calculate the snap points. - state.snapPoints = [ - action.payload, - action.payload / 2, - action.payload / 3, - action.payload / 4 - ] + // Action payload here is the max width of the plots webview. When changed, we re-calculate the max number of plots per row + const maxWidth = action.payload + state.maxNbPlotsPerRow = Math.floor(maxWidth / 300) }, setZoomedInPlot: ( // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -94,7 +88,7 @@ export const { updateHasUnselectedPlots, updateSelectedRevisions, setZoomedInPlot, - setSnapPoints + setMaxNbPlotsPerRow } = webviewSlice.actions export default webviewSlice.reducer diff --git a/webview/src/shared/components/slider/MinMaxSlider.tsx b/webview/src/shared/components/slider/MinMaxSlider.tsx new file mode 100644 index 0000000000..4c0e9fc101 --- /dev/null +++ b/webview/src/shared/components/slider/MinMaxSlider.tsx @@ -0,0 +1,42 @@ +import React, { FormEvent } from 'react' +import styles from './styles.module.scss' + +interface MinMaxSliderProps { + minimum?: number + maximum: number + step?: number + defaultValue: number + label: string + onChange: (newValue: number) => void +} + +export const MinMaxSlider: React.FC = ({ + minimum, + maximum, + step, + defaultValue, + label, + onChange +}) => { + const handleOnChange = (e: FormEvent) => { + onChange(Number.parseFloat(e.currentTarget.value)) + } + + return ( +
+ + +
+ ) +} diff --git a/webview/src/shared/components/slider/styles.module.scss b/webview/src/shared/components/slider/styles.module.scss new file mode 100644 index 0000000000..f5cd7f103e --- /dev/null +++ b/webview/src/shared/components/slider/styles.module.scss @@ -0,0 +1,17 @@ +@import '../../variables.scss'; + +.wrapper { + display: flex; + align-items: center; +} + +.label { + font-size: 0.6rem; + margin-right: 10px; + color: $fg-color; +} + +.slider { + accent-color: $accent-color; + outline: none !important; +} diff --git a/webview/src/shared/components/virtualizedGrid/VirtualizedGrid.tsx b/webview/src/shared/components/virtualizedGrid/VirtualizedGrid.tsx index 47a0f1c9d4..46ac61ac82 100644 --- a/webview/src/shared/components/virtualizedGrid/VirtualizedGrid.tsx +++ b/webview/src/shared/components/virtualizedGrid/VirtualizedGrid.tsx @@ -1,3 +1,4 @@ +import { DEFAULT_NB_ITEMS_PER_REOW } from 'dvc/src/plots/webview/contract' import React, { useRef } from 'react' import { AutoSizer, @@ -7,7 +8,6 @@ import { Grid } from 'react-virtualized' import styles from './styles.module.scss' -import { DEFAULT_NB_ITEMS_PER_ROW } from '../../../plots/components/util' interface VirtualizedGridProps { items: JSX.Element[] @@ -18,7 +18,7 @@ export const OVERSCAN_ROW_COUNT = 15 export const VirtualizedGrid: React.FC = ({ items, - nbItemsPerRow = DEFAULT_NB_ITEMS_PER_ROW + nbItemsPerRow = DEFAULT_NB_ITEMS_PER_REOW }) => { const cache = useRef( new CellMeasurerCache({ diff --git a/webview/src/stories/ComparisonTable.stories.tsx b/webview/src/stories/ComparisonTable.stories.tsx index d14e7b8ef0..f747535879 100644 --- a/webview/src/stories/ComparisonTable.stories.tsx +++ b/webview/src/stories/ComparisonTable.stories.tsx @@ -6,8 +6,8 @@ import React from 'react' import { Provider, useDispatch } from 'react-redux' import { ComparisonRevisionData, - PlotsComparisonData, - PlotNumberOfItemsPerRow + DEFAULT_NB_ITEMS_PER_REOW, + PlotsComparisonData } from 'dvc/src/plots/webview/contract' import comparisonTableFixture from 'dvc/src/test/fixtures/plotsDiff/comparison' import { EXPERIMENT_WORKSPACE_ID } from 'dvc/src/cli/dvc/contract' @@ -44,7 +44,7 @@ const Template: Story = ({ plots, revisions }) => { ({ ...plot, - title: truncateVerticalTitle( - plot.title, - PlotNumberOfItemsPerRow.THREE - ) as string + title: truncateVerticalTitle(plot.title, 3) as string })) } const manyCheckpointPlots = ( length: number, - size = PlotNumberOfItemsPerRow.TWO + size = DEFAULT_NB_ITEMS_PER_REOW ) => Array.from({ length }, () => checkpointPlotsFixture.plots[0]).map( (plot, i) => { @@ -143,7 +140,7 @@ WithTemplateOnly.args = { selectedRevisions: plotsRevisionsFixture, template: { ...templatePlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.TWO + nbItemsPerRow: DEFAULT_NB_ITEMS_PER_REOW } } } @@ -198,21 +195,21 @@ AllLarge.args = { data: { checkpoint: { ...checkpointPlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.ONE + nbItemsPerRow: 1 }, comparison: { ...comparisonPlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.ONE + nbItemsPerRow: 1 }, custom: { ...customPlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.ONE + nbItemsPerRow: 1 }, sectionCollapsed: DEFAULT_SECTION_COLLAPSED, selectedRevisions: plotsRevisionsFixture, template: { ...templatePlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.ONE + nbItemsPerRow: 1 } } } @@ -224,17 +221,17 @@ AllSmall.args = { checkpoint: smallCheckpointPlotsFixture, comparison: { ...comparisonPlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.THREE + nbItemsPerRow: 3 }, custom: { ...customPlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.THREE + nbItemsPerRow: 3 }, sectionCollapsed: DEFAULT_SECTION_COLLAPSED, selectedRevisions: plotsRevisionsFixture, template: { ...templatePlotsFixture, - nbItemsPerRow: PlotNumberOfItemsPerRow.THREE + nbItemsPerRow: 3 } } } diff --git a/webview/src/stories/Slider.stories.tsx b/webview/src/stories/Slider.stories.tsx new file mode 100644 index 0000000000..e71e524959 --- /dev/null +++ b/webview/src/stories/Slider.stories.tsx @@ -0,0 +1,32 @@ +import { Meta, Story } from '@storybook/react/types-6-0' +import React from 'react' +import '../shared/style.scss' +import './test-vscode-styles.scss' +import { DISABLE_CHROMATIC_SNAPSHOTS } from './util' +import { MinMaxSlider } from '../shared/components/slider/MinMaxSlider' + +export default { + args: { + defaultValue: 2, + label: 'Slider with a max', + maximum: 10 + }, + component: MinMaxSlider, + parameters: DISABLE_CHROMATIC_SNAPSHOTS, + title: 'Slider' +} as Meta + +const Template: Story<{ + maximum: number + defaultValue: number + label: string +}> = ({ maximum, defaultValue, label }) => ( + {}} + /> +) + +export const MinMaxSliderWithOnlyMax = Template.bind({}) diff --git a/webview/src/test/mouseEventsWithCoordinates.ts b/webview/src/test/mouseEventsWithCoordinates.ts deleted file mode 100644 index 422f426516..0000000000 --- a/webview/src/test/mouseEventsWithCoordinates.ts +++ /dev/null @@ -1,31 +0,0 @@ -import { act, fireEvent } from '@testing-library/react' - -export const pickAndMove = ( - node: HTMLElement, - xDifference = 0, - yDifference = 0, - cancel?: boolean -) => { - const startingPositionX = xDifference >= 0 ? 0 : -xDifference - const startingPositionY = yDifference >= 0 ? 0 : -yDifference - - act(() => { - fireEvent.mouseDown(node, { - clientX: startingPositionX, - clientY: startingPositionY - }) - }) - if (cancel) { - fireEvent.keyDown(node, { key: 'Escape' }) - } - act(() => { - fireEvent.mouseMove(node, { - clientX: xDifference, - clientY: yDifference - }) - }) - - act(() => { - fireEvent.mouseUp(node) - }) -}