Resize comparison table with slider (#3480)

* Resize comparison tablw with slider

* Add tests

* Fix colliding headers for the comparison table

Author: sroy3

extension/src/persistence/constants.ts

@@ -11,7 +11,7 @@ export enum PersistenceKey {
   PLOT_COMPARISON_PATHS_ORDER = 'plotComparisonPathsOrder',
   PLOT_HEIGHT = 'plotHeight',
   PLOT_METRIC_ORDER = 'plotMetricOrder:',
-  PLOT_NB_ITEMS_PER_ROW = 'plotNbItemsPerRow:',
+  PLOT_NB_ITEMS_PER_ROW_OR_WIDTH = 'plotNbItemsPerRowOrWidth:',
   PLOTS_CUSTOM_ORDER = 'plotCustomOrder:',
   PLOT_SECTION_COLLAPSED = 'plotSectionCollapsed:',
   PLOT_SELECTED_METRICS = 'plotSelectedMetrics:',

extension/src/persistence/util.test.ts

@@ -4,7 +4,7 @@ import { resetPersistedState } from './util'
 import { buildMockMemento } from '../test/util'
 import {
   DEFAULT_HEIGHT,
-  DEFAULT_SECTION_NB_ITEMS_PER_ROW
+  DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH
 } from '../plots/webview/contract'
 
 jest.mock('vscode')
@@ -31,8 +31,8 @@ describe('Persistence util', () => {
     it('should reset all values from all dvc roots', async () => {
       const persistedState = {
         [PersistenceKey.PLOT_HEIGHT + 'root1']: DEFAULT_HEIGHT,
-        [PersistenceKey.PLOT_NB_ITEMS_PER_ROW + 'root2']:
-          DEFAULT_SECTION_NB_ITEMS_PER_ROW
+        [PersistenceKey.PLOT_NB_ITEMS_PER_ROW_OR_WIDTH + 'root2']:
+          DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH
       }
       const workspaceState = buildMockMemento(persistedState)
 
@@ -42,7 +42,9 @@ describe('Persistence util', () => {
         workspaceState.get(PersistenceKey.PLOT_HEIGHT + 'root1')
       ).toBeUndefined()
       expect(
-        workspaceState.get(PersistenceKey.PLOT_NB_ITEMS_PER_ROW + 'root2')
+        workspaceState.get(
+          PersistenceKey.PLOT_NB_ITEMS_PER_ROW_OR_WIDTH + 'root2'
+        )
       ).toBeUndefined()
     })
   })

extension/src/plots/model/index.test.ts

@@ -2,7 +2,7 @@ import { PlotsModel } from '.'
 import {
   DEFAULT_NB_ITEMS_PER_ROW,
   DEFAULT_SECTION_COLLAPSED,
-  DEFAULT_SECTION_NB_ITEMS_PER_ROW,
+  DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH,
   PlotsSection
 } from '../webview/contract'
 import { buildMockMemento } from '../../test/util'
@@ -25,8 +25,8 @@ describe('plotsModel', () => {
   const memento = buildMockMemento({
     [PersistenceKey.PLOT_SELECTED_METRICS + exampleDvcRoot]:
       persistedSelectedMetrics,
-    [PersistenceKey.PLOT_NB_ITEMS_PER_ROW + exampleDvcRoot]:
-      DEFAULT_SECTION_NB_ITEMS_PER_ROW
+    [PersistenceKey.PLOT_NB_ITEMS_PER_ROW_OR_WIDTH + exampleDvcRoot]:
+      DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH
   })
   const mockedGetSelectedRevisions = jest.fn()
   const mockedGetFirstThreeColumnOrder = jest.fn()
@@ -68,27 +68,27 @@ describe('plotsModel', () => {
   })
 
   it('should change the plotSize when calling setPlotSize', () => {
-    expect(model.getNbItemsPerRow(PlotsSection.CHECKPOINT_PLOTS)).toStrictEqual(
-      DEFAULT_NB_ITEMS_PER_ROW
-    )
+    expect(
+      model.getNbItemsPerRowOrWidth(PlotsSection.CHECKPOINT_PLOTS)
+    ).toStrictEqual(DEFAULT_NB_ITEMS_PER_ROW)
 
-    model.setNbItemsPerRow(PlotsSection.CHECKPOINT_PLOTS, 1)
+    model.setNbItemsPerRowOrWidth(PlotsSection.CHECKPOINT_PLOTS, 1)
 
-    expect(model.getNbItemsPerRow(PlotsSection.CHECKPOINT_PLOTS)).toStrictEqual(
-      1
-    )
+    expect(
+      model.getNbItemsPerRowOrWidth(PlotsSection.CHECKPOINT_PLOTS)
+    ).toStrictEqual(1)
   })
 
   it('should update the persisted plot size when calling setPlotSize', () => {
     const mementoUpdateSpy = jest.spyOn(memento, 'update')
 
-    model.setNbItemsPerRow(PlotsSection.CHECKPOINT_PLOTS, 2)
+    model.setNbItemsPerRowOrWidth(PlotsSection.CHECKPOINT_PLOTS, 2)
 
     expect(mementoUpdateSpy).toHaveBeenCalledTimes(1)
     expect(mementoUpdateSpy).toHaveBeenCalledWith(
-      PersistenceKey.PLOT_NB_ITEMS_PER_ROW + exampleDvcRoot,
+      PersistenceKey.PLOT_NB_ITEMS_PER_ROW_OR_WIDTH + exampleDvcRoot,
       {
-        ...DEFAULT_SECTION_NB_ITEMS_PER_ROW,
+        ...DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH,
         [PlotsSection.CHECKPOINT_PLOTS]: 2
       }
     )

extension/src/plots/model/index.ts

@@ -23,7 +23,7 @@ import {
   Revision,
   ComparisonRevisionData,
   DEFAULT_SECTION_COLLAPSED,
-  DEFAULT_SECTION_NB_ITEMS_PER_ROW,
+  DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH,
   PlotsSection,
   SectionCollapsed,
   CustomPlotData,
@@ -56,7 +56,7 @@ export type CustomPlotsOrderValue = { metric: string; param: string }
 export class PlotsModel extends ModelWithPersistence {
   private readonly experiments: Experiments
 
-  private nbItemsPerRow: Record<PlotsSection, number>
+  private nbItemsPerRowOrWidth: Record<PlotsSection, number>
   private height: Record<PlotsSection, PlotHeight>
   private customPlotsOrder: CustomPlotsOrderValue[]
   private sectionCollapsed: SectionCollapsed
@@ -85,9 +85,9 @@ export class PlotsModel extends ModelWithPersistence {
     super(dvcRoot, workspaceState)
     this.experiments = experiments
 
-    this.nbItemsPerRow = this.revive(
-      PersistenceKey.PLOT_NB_ITEMS_PER_ROW,
-      DEFAULT_SECTION_NB_ITEMS_PER_ROW
+    this.nbItemsPerRowOrWidth = this.revive(
+      PersistenceKey.PLOT_NB_ITEMS_PER_ROW_OR_WIDTH,
+      DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH
     )
     this.height = this.revive(PersistenceKey.PLOT_HEIGHT, DEFAULT_HEIGHT)
 
@@ -183,7 +183,9 @@ export class PlotsModel extends ModelWithPersistence {
     return {
       colors,
       height: this.getHeight(PlotsSection.CHECKPOINT_PLOTS),
-      nbItemsPerRow: this.getNbItemsPerRow(PlotsSection.CHECKPOINT_PLOTS),
+      nbItemsPerRow: this.getNbItemsPerRowOrWidth(
+        PlotsSection.CHECKPOINT_PLOTS
+      ),
       plots: this.getPlots(this.checkpointPlots, selectedExperiments),
       selectedMetrics: this.getSelectedMetrics()
     }
@@ -195,7 +197,7 @@ export class PlotsModel extends ModelWithPersistence {
     }
     return {
       height: this.getHeight(PlotsSection.CUSTOM_PLOTS),
-      nbItemsPerRow: this.getNbItemsPerRow(PlotsSection.CUSTOM_PLOTS),
+      nbItemsPerRow: this.getNbItemsPerRowOrWidth(PlotsSection.CUSTOM_PLOTS),
       plots: this.customPlots
     }
   }
@@ -403,14 +405,17 @@ export class PlotsModel extends ModelWithPersistence {
     this.persist(PersistenceKey.PLOT_METRIC_ORDER, this.metricOrder)
   }
 
-  public setNbItemsPerRow(section: PlotsSection, nbItemsPerRow: number) {
-    this.nbItemsPerRow[section] = nbItemsPerRow
-    this.persist(PersistenceKey.PLOT_NB_ITEMS_PER_ROW, this.nbItemsPerRow)
+  public setNbItemsPerRowOrWidth(section: PlotsSection, nbItemsPerRow: number) {
+    this.nbItemsPerRowOrWidth[section] = nbItemsPerRow
+    this.persist(
+      PersistenceKey.PLOT_NB_ITEMS_PER_ROW_OR_WIDTH,
+      this.nbItemsPerRowOrWidth
+    )
   }
 
-  public getNbItemsPerRow(section: PlotsSection) {
-    if (this.nbItemsPerRow[section]) {
-      return this.nbItemsPerRow[section]
+  public getNbItemsPerRowOrWidth(section: PlotsSection) {
+    if (this.nbItemsPerRowOrWidth[section]) {
+      return this.nbItemsPerRowOrWidth[section]
     }
     return DEFAULT_NB_ITEMS_PER_ROW
   }
@@ -511,7 +516,7 @@ export class PlotsModel extends ModelWithPersistence {
           id,
           title: truncateVerticalTitle(
             id,
-            this.getNbItemsPerRow(PlotsSection.CHECKPOINT_PLOTS),
+            this.getNbItemsPerRowOrWidth(PlotsSection.CHECKPOINT_PLOTS),
             this.getHeight(PlotsSection.CHECKPOINT_PLOTS)
           ) as string,
           values: values.filter(value =>
@@ -563,7 +568,7 @@ export class PlotsModel extends ModelWithPersistence {
       selectedRevisions.map(({ revision }) => revision),
       this.templates,
       this.revisionData,
-      this.getNbItemsPerRow(PlotsSection.TEMPLATE_PLOTS),
+      this.getNbItemsPerRowOrWidth(PlotsSection.TEMPLATE_PLOTS),
       this.getHeight(PlotsSection.TEMPLATE_PLOTS),
       this.getRevisionColors(selectedRevisions),
       this.multiSourceEncoding

extension/src/plots/webview/contract.ts

@@ -14,17 +14,19 @@ export enum PlotHeight {
 
 export const DEFAULT_PLOT_HEIGHT = PlotHeight.SMALL
 
+export const DEFAULT_PLOT_WIDTH = 2
+
 export enum PlotsSection {
   CHECKPOINT_PLOTS = 'checkpoint-plots',
   TEMPLATE_PLOTS = 'template-plots',
   COMPARISON_TABLE = 'comparison-table',
   CUSTOM_PLOTS = 'custom-plots'
 }
 
-export const DEFAULT_SECTION_NB_ITEMS_PER_ROW = {
+export const DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH = {
   [PlotsSection.CHECKPOINT_PLOTS]: DEFAULT_NB_ITEMS_PER_ROW,
   [PlotsSection.TEMPLATE_PLOTS]: DEFAULT_NB_ITEMS_PER_ROW,
-  [PlotsSection.COMPARISON_TABLE]: DEFAULT_NB_ITEMS_PER_ROW,
+  [PlotsSection.COMPARISON_TABLE]: DEFAULT_PLOT_WIDTH,
   [PlotsSection.CUSTOM_PLOTS]: DEFAULT_NB_ITEMS_PER_ROW
 }
 
@@ -69,7 +71,7 @@ export type Revision = {
 
 export interface PlotsComparisonData {
   plots: ComparisonPlots
-  nbItemsPerRow: number
+  width: number
   height: PlotHeight
   revisions: Revision[]
 }

extension/src/plots/webview/messages.ts

@@ -136,7 +136,7 @@ export class WebviewMessages {
     nbItemsPerRow: number,
     height: PlotHeight
   ) {
-    this.plots.setNbItemsPerRow(section, nbItemsPerRow)
+    this.plots.setNbItemsPerRowOrWidth(section, nbItemsPerRow)
     this.plots.setHeight(section, height)
     sendTelemetryEvent(
       EventName.VIEWS_PLOTS_SECTION_RESIZED,
@@ -372,7 +372,9 @@ export class WebviewMessages {
 
     return {
       height: this.plots.getHeight(PlotsSection.TEMPLATE_PLOTS),
-      nbItemsPerRow: this.plots.getNbItemsPerRow(PlotsSection.TEMPLATE_PLOTS),
+      nbItemsPerRow: this.plots.getNbItemsPerRowOrWidth(
+        PlotsSection.TEMPLATE_PLOTS
+      ),
       plots
     }
   }
@@ -389,11 +391,11 @@ export class WebviewMessages {
 
     return {
       height: this.plots.getHeight(PlotsSection.COMPARISON_TABLE),
-      nbItemsPerRow: this.plots.getNbItemsPerRow(PlotsSection.COMPARISON_TABLE),
       plots: comparison.map(({ path, revisions }) => {
         return { path, revisions: this.getRevisionsWithCorrectUrls(revisions) }
       }),
-      revisions: overrideRevs || this.plots.getComparisonRevisions()
+      revisions: overrideRevs || this.plots.getComparisonRevisions(),
+      width: this.plots.getNbItemsPerRowOrWidth(PlotsSection.COMPARISON_TABLE)
     }
   }
 

extension/src/test/fixtures/plotsDiff/index.ts

@@ -13,8 +13,9 @@ import {
   TemplatePlots,
   Revision,
   PlotsComparisonData,
+  DEFAULT_PLOT_HEIGHT,
   DEFAULT_NB_ITEMS_PER_ROW,
-  DEFAULT_PLOT_HEIGHT
+  DEFAULT_PLOT_WIDTH
 } from '../../../plots/webview/contract'
 import { join } from '../../util/path'
 import { copyOriginalColors } from '../../../experiments/model/status/colors'
@@ -706,7 +707,7 @@ export const getComparisonWebviewMessage = (
   return {
     revisions: getRevisions(),
     plots: plotAcc,
-    nbItemsPerRow: DEFAULT_NB_ITEMS_PER_ROW,
+    width: DEFAULT_PLOT_WIDTH,
     height: DEFAULT_PLOT_HEIGHT
   }
 }

extension/src/test/suite/plots/index.test.ts

@@ -242,9 +242,10 @@ suite('Plots Test Suite', () => {
       const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent')
       const mockMessageReceived = getMessageReceivedEmitter(webview)
 
-      const mockSetPlotSize = stub(plotsModel, 'setNbItemsPerRow').returns(
-        undefined
-      )
+      const mockSetPlotSize = stub(
+        plotsModel,
+        'setNbItemsPerRowOrWidth'
+      ).returns(undefined)
 
       mockMessageReceived.fire({
         payload: {

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

@@ -28,8 +28,8 @@ import {
   PlotsSection,
   TemplatePlotGroup,
   TemplatePlotsData,
-  DEFAULT_NB_ITEMS_PER_ROW,
-  DEFAULT_PLOT_HEIGHT
+  DEFAULT_PLOT_HEIGHT,
+  DEFAULT_NB_ITEMS_PER_ROW
 } from 'dvc/src/plots/webview/contract'
 import {
   MessageFromWebviewType,
@@ -264,7 +264,6 @@ describe('App', () => {
       checkpoint: null,
       comparison: {
         height: DEFAULT_PLOT_HEIGHT,
-        nbItemsPerRow: DEFAULT_NB_ITEMS_PER_ROW,
         plots: [
           {
             path: 'training/plots/images/misclassified.jpg',
@@ -280,7 +279,8 @@ describe('App', () => {
             id: 'ad2b5ec854a447d00d9dfa9cdf88211a39a17813',
             revision: 'ad2b5ec'
           }
-        ]
+        ],
+        width: DEFAULT_NB_ITEMS_PER_ROW
       },
       hasPlots: true,
       hasUnselectedPlots: false,
@@ -713,22 +713,66 @@ describe('App', () => {
     expect(screen.queryByTestId('size-sliders')).not.toBeInTheDocument()
   })
 
-  it('should not display a slider to pick the number of items per row if the action is unavailable', () => {
+  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({
-      comparison: comparisonTableFixture
+      checkpoint: checkpointPlotsFixture
     })
-    setWrapperSize(store)
+    setWrapperSize(store, 400)
 
     expect(screen.queryByTestId('size-sliders')).not.toBeInTheDocument()
   })
 
-  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', () => {
+  it('should display both size sliders for checkpoint plots', () => {
     const store = renderAppWithOptionalData({
       checkpoint: checkpointPlotsFixture
     })
-    setWrapperSize(store, 400)
+    setWrapperSize(store)
 
-    expect(screen.queryByTestId('size-sliders')).not.toBeInTheDocument()
+    const plotResizers = within(
+      screen.getByTestId('size-sliders')
+    ).getAllByRole('slider')
+
+    expect(plotResizers.length).toBe(2)
+  })
+
+  it('should display both size sliders for template plots', () => {
+    const store = renderAppWithOptionalData({
+      template: templatePlotsFixture
+    })
+    setWrapperSize(store)
+
+    const plotResizers = within(
+      screen.getByTestId('size-sliders')
+    ).getAllByRole('slider')
+
+    expect(plotResizers.length).toBe(2)
+  })
+
+  it('should display both size sliders for custom plots', () => {
+    const store = renderAppWithOptionalData({
+      custom: customPlotsFixture,
+      template: templatePlotsFixture
+    })
+    setWrapperSize(store)
+
+    const plotResizers = within(
+      screen.getAllByTestId('size-sliders')[1]
+    ).getAllByRole('slider')
+
+    expect(plotResizers.length).toBe(2)
+  })
+
+  it('should not display the height slider for the comparison table', () => {
+    const store = renderAppWithOptionalData({
+      comparison: comparisonTableFixture
+    })
+    setWrapperSize(store)
+
+    const plotResizers = within(
+      screen.getByTestId('size-sliders')
+    ).getAllByRole('slider')
+
+    expect(plotResizers.length).toBe(1)
   })
 
   it('should send a message to the extension with the selected size when changing the width of plots', () => {