Skip to content
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

Calculate the default nbOrItemsPerRow from the screen size #3492

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions extension/src/plots/model/collect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,7 @@ const collectTemplatePlot = (
path: string,
template: string,
revisionData: RevisionData,
nbItemsPerRow: number,
nbItemsPerRow: number | undefined,
height: number,
revisionColors: ColorScale | undefined,
multiSourceEncoding: MultiSourceEncoding
Expand Down Expand Up @@ -630,7 +630,7 @@ const collectTemplateGroup = (
selectedRevisions: string[],
templates: TemplateAccumulator,
revisionData: RevisionData,
nbItemsPerRow: number,
nbItemsPerRow: number | undefined,
height: number,
revisionColors: ColorScale | undefined,
multiSourceEncoding: MultiSourceEncoding
Expand Down Expand Up @@ -663,7 +663,7 @@ export const collectSelectedTemplatePlots = (
selectedRevisions: string[],
templates: TemplateAccumulator,
revisionData: RevisionData,
nbItemsPerRow: number,
nbItemsPerRow: number | undefined,
height: number,
revisionColors: ColorScale | undefined,
multiSourceEncoding: MultiSourceEncoding
Expand Down
2 changes: 1 addition & 1 deletion extension/src/plots/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export type CustomPlotsOrderValue = { metric: string; param: string }
export class PlotsModel extends ModelWithPersistence {
private readonly experiments: Experiments

private nbItemsPerRowOrWidth: Record<PlotsSection, number>
private nbItemsPerRowOrWidth: Record<PlotsSection, number | undefined>
private height: Record<PlotsSection, PlotHeight>
private customPlotsOrder: CustomPlotsOrderValue[]
private sectionCollapsed: SectionCollapsed
Expand Down
19 changes: 4 additions & 15 deletions extension/src/plots/vega/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@ import smoothTemplate from '../../test/fixtures/plotsDiff/templates/smooth'
import multiSourceTemplate from '../../test/fixtures/plotsDiff/templates/multiSource'
import { copyOriginalColors } from '../../experiments/model/status/colors'
import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract'
import {
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT
} from '../webview/contract'
import { DEFAULT_PLOT_HEIGHT } from '../webview/contract'

describe('isMultiViewPlot', () => {
it('should recognize the confusion matrix template as a multi view plot', () => {
Expand Down Expand Up @@ -87,11 +84,7 @@ describe('getColorScale', () => {

describe('extendVegaSpec', () => {
it('should not add encoding if no color scale is provided', () => {
const extendedSpec = extendVegaSpec(
linearTemplate,
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT
)
const extendedSpec = extendVegaSpec(linearTemplate, 2, DEFAULT_PLOT_HEIGHT)
expect(extendedSpec.encoding).toBeUndefined()
})

Expand All @@ -102,7 +95,7 @@ describe('extendVegaSpec', () => {
}
const extendedSpec = extendVegaSpec(
linearTemplate,
DEFAULT_NB_ITEMS_PER_ROW,
2,
DEFAULT_PLOT_HEIGHT,
{
color: colorScale
Expand Down Expand Up @@ -174,11 +167,7 @@ describe('extendVegaSpec', () => {

it('should truncate all titles from the left to 50 characters for regular plots', () => {
const spec = withLongTemplatePlotTitle()
const updatedSpec = extendVegaSpec(
spec,
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT
)
const updatedSpec = extendVegaSpec(spec, 2, DEFAULT_PLOT_HEIGHT)

const truncatedTitle = '…-many-many-characters-at-least-seventy-characters'
const truncatedHorizontalTitle =
Expand Down
12 changes: 6 additions & 6 deletions extension/src/plots/vega/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, DEFAULT_NB_ITEMS_PER_ROW } from '../webview/contract'
import { ColorScale } from '../webview/contract'
import { ShapeEncoding, StrokeDashEncoding } from '../multiSource/constants'
import { Color } from '../../experiments/model/status/colors'

Expand Down Expand Up @@ -255,16 +255,16 @@ const truncateTitle = (

export const truncateVerticalTitle = (
title: Text | Title,
width: number,
height: number
width = 2,
height = 2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was great having this on the extension side, but since we rely so much on screen size now. I think it'll be best to move this back on the webview side.

) => truncateTitle(title, Math.floor((50 - (width - height) * 5) * 0.75))

const isEndValue = (valueType: string) =>
['string', 'number', 'boolean'].includes(valueType)

export const truncateTitles = (
spec: TopLevelSpec,
width: number,
width = 2,
height: number,
vertical?: boolean
// eslint-disable-next-line sonarjs/cognitive-complexity
Expand All @@ -286,7 +286,7 @@ export const truncateTitles = (
const title = value as unknown as Title
specCopy[key] = vertical
? truncateVerticalTitle(title, width, height)
: truncateTitle(title, width > DEFAULT_NB_ITEMS_PER_ROW ? 30 : 50)
: truncateTitle(title, width > 2 ? 30 : 50)
} else if (isEndValue(valueType)) {
specCopy[key] = value
} else if (Array.isArray(value)) {
Expand All @@ -306,7 +306,7 @@ export const truncateTitles = (

export const extendVegaSpec = (
spec: TopLevelSpec,
width: number,
width: number | undefined,
height: number,
encoding?: {
color?: ColorScale
Expand Down
10 changes: 5 additions & 5 deletions extension/src/plots/webview/contract.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { VisualizationSpec } from 'react-vega'
import { Color } from '../../experiments/model/status/colors'

export const DEFAULT_NB_ITEMS_PER_ROW = 2
export const DEFAULT_NB_ITEMS_PER_ROW = undefined

export enum PlotHeight {
SMALLER,
Expand Down Expand Up @@ -71,7 +71,7 @@ export type Revision = {

export interface PlotsComparisonData {
plots: ComparisonPlots
width: number
width: number | undefined
height: PlotHeight
revisions: Revision[]
}
Expand Down Expand Up @@ -104,7 +104,7 @@ export type CustomPlotData = {

export type CustomPlotsData = {
plots: CustomPlotData[]
nbItemsPerRow: number
nbItemsPerRow: number | undefined
height: PlotHeight
}

Expand All @@ -113,7 +113,7 @@ export type CheckpointPlotData = CheckpointPlot & { title: string }
export type CheckpointPlotsData = {
plots: CheckpointPlotData[]
colors: ColorScale
nbItemsPerRow: number
nbItemsPerRow: number | undefined
height: PlotHeight
selectedMetrics?: string[]
}
Expand Down Expand Up @@ -162,7 +162,7 @@ export type TemplatePlotSection = {

export interface TemplatePlotsData {
plots: TemplatePlotSection[]
nbItemsPerRow: number
nbItemsPerRow: number | undefined
height: PlotHeight
}

Expand Down
2 changes: 1 addition & 1 deletion extension/src/test/fixtures/plotsDiff/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ const extendedSpecs = (plotsOutput: TemplatePlots): TemplatePlotSection[] => {
) || []
}
} as TopLevelSpec,
DEFAULT_NB_ITEMS_PER_ROW,
2,
DEFAULT_PLOT_HEIGHT,
{
color: {
Expand Down
3 changes: 1 addition & 2 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ import * as FileSystem from '../../../fileSystem'
import * as ProcessExecution from '../../../process/execution'
import { DvcReader } from '../../../cli/dvc/reader'
import { DvcViewer } from '../../../cli/dvc/viewer'
import { DEFAULT_NB_ITEMS_PER_ROW } from '../../../plots/webview/contract'

const { openFileInEditor } = FileSystem

Expand Down Expand Up @@ -338,7 +337,7 @@ suite('Experiments Test Suite', () => {
).returns(undefined)

const mockColumnId = 'params:params.yaml:lr'
const mockWidth = DEFAULT_NB_ITEMS_PER_ROW
const mockWidth = 2

mockMessageReceived.fire({
payload: { id: mockColumnId, width: mockWidth },
Expand Down
10 changes: 5 additions & 5 deletions webview/src/plots/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1764,7 +1764,7 @@ describe('App', () => {
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(15) },
DEFAULT_NB_ITEMS_PER_ROW,
2,
PlotsSection.CHECKPOINT_PLOTS
)

Expand All @@ -1774,7 +1774,7 @@ describe('App', () => {
it('should not wrap the checkpoint plots in a big grid (virtualize them) when there are fourteen regular plots', async () => {
await renderAppAndChangeSize(
{ checkpoint: createCheckpointPlots(14) },
DEFAULT_NB_ITEMS_PER_ROW,
2,
PlotsSection.CHECKPOINT_PLOTS
)

Expand All @@ -1784,7 +1784,7 @@ describe('App', () => {
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(15) },
DEFAULT_NB_ITEMS_PER_ROW,
2,
PlotsSection.TEMPLATE_PLOTS
)

Expand All @@ -1794,7 +1794,7 @@ describe('App', () => {
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(14) },
DEFAULT_NB_ITEMS_PER_ROW,
2,
PlotsSection.TEMPLATE_PLOTS
)

Expand All @@ -1808,7 +1808,7 @@ describe('App', () => {
beforeEach(async () => {
store = await renderAppAndChangeSize(
{ checkpoint },
DEFAULT_NB_ITEMS_PER_ROW,
2,
PlotsSection.CHECKPOINT_PLOTS
)
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import styles from '../styles.module.scss'
import { withScale } from '../../../util/styles'
import { plotDataStore } from '../plotDataStore'
import { PlotsState } from '../../store'
import { useNbOfItemsPerRow } from '../../hooks/useNumberOfItemsPerRowOrWidth'

interface CheckpointPlotProps {
id: string
Expand All @@ -27,6 +28,7 @@ export const CheckpointPlot: React.FC<CheckpointPlotProps> = ({
const nbItemsPerRow = useSelector(
(state: PlotsState) => state.checkpoint.nbItemsPerRow
)
const nbItemsPerRowOrDefault = useNbOfItemsPerRow(nbItemsPerRow)

const spec = useMemo(() => {
const title = plot?.title
Expand All @@ -52,7 +54,7 @@ export const CheckpointPlot: React.FC<CheckpointPlotProps> = ({
spec={spec}
id={id}
changeDisabledDragIds={changeDisabledDragIds}
currentSnapPoint={nbItemsPerRow}
currentSnapPoint={nbItemsPerRowOrDefault}
section={PlotsSection.CHECKPOINT_PLOTS}
/>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { shouldUseVirtualizedGrid } from '../util'
import { PlotsState } from '../../store'
import { changeOrderWithDraggedInfo } from '../../../util/array'
import { LoadingSection, sectionIsLoading } from '../LoadingSection'
import { useNbOfItemsPerRow } from '../../hooks/useNumberOfItemsPerRowOrWidth'

interface CheckpointPlotsProps {
plotsIds: string[]
Expand All @@ -32,6 +33,7 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({
const { nbItemsPerRow, hasData, disabledDragPlotIds } = useSelector(
(state: PlotsState) => state.checkpoint
)
const nbItemsPerRowOrDefault = useNbOfItemsPerRow(nbItemsPerRow)
const [onSection, setOnSection] = useState(false)
const draggedRef = useSelector(
(state: PlotsState) => state.dragAndDrop.draggedRef
Expand Down Expand Up @@ -69,7 +71,7 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({

const useVirtualizedGrid = shouldUseVirtualizedGrid(
items.length,
nbItemsPerRow
nbItemsPerRowOrDefault
)

const handleDropAtTheEnd = () => {
Expand Down Expand Up @@ -104,7 +106,7 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({
useVirtualizedGrid
? {
component: VirtualizedGrid as React.FC<WrapperProps>,
props: { nbItemsPerRow }
props: { nbItemsPerRow: nbItemsPerRowOrDefault }
}
: undefined
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { changeSize } from './checkpointPlotsSlice'
import { PlotsContainer } from '../PlotsContainer'
import { sendMessage } from '../../../shared/vscode'
import { PlotsState } from '../../store'
import { useNbOfItemsPerRow } from '../../hooks/useNumberOfItemsPerRowOrWidth'

export const CheckpointPlotsWrapper: React.FC = () => {
const {
Expand All @@ -19,6 +20,7 @@ export const CheckpointPlotsWrapper: React.FC = () => {
} = useSelector((state: PlotsState) => state.checkpoint)
const [metrics, setMetrics] = useState<string[]>([])
const [selectedPlots, setSelectedPlots] = useState<string[]>([])
const nbItemsPerRowOrDefault = useNbOfItemsPerRow(nbItemsPerRow)

useEffect(() => {
setMetrics([...plotsIds].sort())
Expand Down Expand Up @@ -48,7 +50,7 @@ export const CheckpointPlotsWrapper: React.FC = () => {
title="Trends"
sectionKey={PlotsSection.CHECKPOINT_PLOTS}
menu={menu}
nbItemsPerRowOrWidth={nbItemsPerRow}
nbItemsPerRowOrWidth={nbItemsPerRowOrDefault}
sectionCollapsed={isCollapsed}
changeSize={changeSize}
hasItems={hasItems}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ import React from 'react'
import { Revision } from 'dvc/src/plots/webview/contract'
import { act } from 'react-dom/test-utils'
import { ComparisonTable } from './ComparisonTable'
import { comparisonTableInitialState } from './comparisonTableSlice'
import {
comparisonTableInitialState,
ComparisonTableState
} from './comparisonTableSlice'
import {
createBubbledEvent,
dragAndDrop,
Expand Down Expand Up @@ -73,7 +76,7 @@ describe('ComparisonTable', () => {
comparison: {
...comparisonTableInitialState,
...props
},
} as ComparisonTableState,
webview: {
...webviewInitialState,
zoomedInPlot: undefined
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const ComparisonTableWrapper: React.FC = () => {
<PlotsContainer
title="Images"
sectionKey={PlotsSection.COMPARISON_TABLE}
nbItemsPerRowOrWidth={width}
nbItemsPerRowOrWidth={width || 2}
changeSize={changeSize}
sectionCollapsed={isCollapsed}
height={height}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { createSlice, PayloadAction } from '@reduxjs/toolkit'
import {
DEFAULT_HEIGHT,
DEFAULT_PLOT_WIDTH,
DEFAULT_SECTION_COLLAPSED,
DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH,
PlotsComparisonData,
Expand All @@ -11,6 +12,7 @@ export interface ComparisonTableState extends PlotsComparisonData {
isCollapsed: boolean
hasData: boolean
rowHeight: number
width: number
}

export const DEFAULT_ROW_HEIGHT = 200
Expand All @@ -23,7 +25,8 @@ export const comparisonTableInitialState: ComparisonTableState = {
revisions: [],
rowHeight: DEFAULT_ROW_HEIGHT,
width:
DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH[PlotsSection.COMPARISON_TABLE]
DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH[PlotsSection.COMPARISON_TABLE] ||
DEFAULT_PLOT_WIDTH
}

export const comparisonTableSlice = createSlice({
Expand All @@ -49,7 +52,8 @@ export const comparisonTableSlice = createSlice({
return {
...state,
...action.payload,
hasData: !!action.payload
hasData: !!action.payload,
width: action.payload.width || DEFAULT_PLOT_WIDTH
}
}
}
Expand Down
Loading