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

Author: sroy3

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
       }
     )
   })

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) {

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()

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)) {

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)

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
 }
 

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
 }
 

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
   }
 }

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 },