Bump min DVC version to 3.33.0 (standardise plots across products) #4734

mattseddon · 2023-09-28T06:33:51Z

This PR makes the changes required to accept the new templates provided by dvc-render in iterative/dvc-render#142. Made available through dvc in iterative/dvc#9931.

The main difference in the approach is that extra anchors have been added to the default templates. These anchors will all be filled in the webview so that the values can be manipulated depending on whether or not the plot is being shown in the zoom modal. We will no longer be overwriting information in templates that do not have the appropriate anchors.

I have consolidated our plot's test fixtures as part of the change. I have been able to drop a few files as the bulk of the complexity has been moved into dvc-render.

Demo

Screen.Recording.2023-11-27.at.12.14.35.pm.mov

mattseddon · 2023-09-28T06:35:24Z

extension/src/cli/dvc/contract.ts

@@ -1,5 +1,3 @@
-import { Plot } from '../../plots/webview/contract'
-
 export const MIN_CLI_VERSION = '2.58.1'


[I] Will need bumped

Don't forget to remove the code that checks the CLI version when running exp rename

Are these things that need to be done before merging?

extension/src/cli/dvc/contract.ts

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

mattseddon · 2023-09-28T06:37:59Z

extension/src/plots/multiSource/collect.ts

 >

-export const mergeFields = (fields: string[]): string =>


[F] All of this logic has moved to dvc-render and the templates.

extension/src/plots/vega/util.ts

mattseddon · 2023-11-26T23:32:02Z

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

@@ -2798,172 +2777,6 @@ describe('App', () => {
      expect(clickEvent.stopPropagation).toHaveBeenCalledTimes(1)
    })

-    it('should have a tooltip on the plot if the title is cut', () => {


[F] See this

mattseddon · 2023-11-26T23:33:44Z

webview/src/plots/components/ZoomedInPlot.tsx

@@ -84,7 +69,9 @@ export const ZoomedInPlot: React.FC<ZoomedInPlotProps> = ({
          ThemeProperty.FONT
        ])

-        const fullThemedSvg = preventSvgTruncation(themedSvg)
+        const fullThemedSvg = addExportBackgroundColor(


[F] This was broken

codeclimate

The PR diff size of 462453 lines exceeds the maximum allowed for the inline comments feature.

sroy3 · 2023-11-27T14:05:47Z

extension/src/cli/dvc/contract.ts

@@ -1,5 +1,3 @@
-import { Plot } from '../../plots/webview/contract'
-
 export const MIN_CLI_VERSION = '2.58.1'


Are these things that need to be done before merging?

sroy3 · 2023-11-27T14:07:27Z

extension/src/cli/dvc/contract.ts

+export const PLOT_ZOOM_AND_PAN_ANCHOR = '<DVC_METRIC_ZOOM_AND_PAN>' as const
+export const PLOT_PARAM_TYPE_ANCHOR = '<DVC_PARAM_TYPE>' as const
+export const PLOT_HEIGHT_ANCHOR = '<DVC_METRIC_PLOT_HEIGHT>' as const
+export const PLOT_WIDTH_ANCHOR = '<DVC_METRIC_PLOT_WIDTH>' as const


Should we have these inside an enum / const? So we can have all tags while importing only one constant (autocompletion as well).

sroy3 · 2023-11-27T14:44:52Z

webview/src/plots/components/ZoomablePlot.tsx

@@ -71,49 +47,45 @@ export const ZoomablePlot: React.FC<ZoomablePlotProps> = ({
  }

  return (
-    <ZoomablePlotWrapper spec={plotProps.spec as SpecWithTitles}>


Are we getting rid of the tooltip?

Got my answer a few files below (https://github.com/iterative/vscode-dvc/pull/4734/files#r1405504743)

sroy3 · 2023-11-27T14:47:25Z

webview/src/plots/components/styles.module.scss

+
+  table {
+    table-layout: auto !important;
+


Was this needed after all? (table-layout)

sroy3 · 2023-11-27T14:48:57Z

webview/src/plots/components/vegaLite/util.ts

+  anchorDefinitions: AnchorDefinitions
+  // eslint-disable-next-line sonarjs/cognitive-complexity
+): unknown => {
+  if (!value) {


Do we want to return early if value === 0?

Since it is also tested on line 38 and value is returned in the end as well, we should remove that check and let isAnchor return false if there is no value.

webview/src/plots/components/vegaLite/util.ts

sroy3 · 2023-11-27T14:54:16Z

webview/src/plots/components/vegaLite/util.ts

+  const updatedAnchors: Record<string, unknown> = {}
+
+  const titleWidth = getMaxHorizontalTitleCharacters(nbItemsPerRow)
+  const titleHeight = getMaxVerticalTitleCharacters(


Since we are inside the webview, we can now calculate the available space and have it looking right all the time.

julieg18

Great work! Should we create a todo list of what needs to be done before this pr gets merged to ensure nothing gets missed?

webview/src/plots/components/vegaLite/util.ts

julieg18 · 2023-11-28T02:41:22Z

extension/src/plots/model/custom.ts

  type === 'number' ? 'quantitative' : 'nominal'

-export const createSpec = (


Since this function no longer takes arguments, should we turn it into an object instead?

codeclimate

The PR diff size of 462453 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 462456 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 462329 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 462329 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 462333 lines exceeds the maximum allowed for the inline comments feature.

codeclimate · 2023-12-06T00:54:59Z

Code Climate has analyzed commit 7bde398 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	2

The test coverage on the diff in this pull request is 98.3% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.2% (0.0% change).

View more on Code Climate.

mattseddon added the product PR that affects product label Sep 28, 2023

mattseddon self-assigned this Sep 28, 2023

mattseddon force-pushed the use-new-templates branch from 2ce1d9e to d2985d1 Compare September 28, 2023 06:34

mattseddon commented Sep 28, 2023

View reviewed changes

extension/src/cli/dvc/contract.ts Outdated Show resolved Hide resolved

mattseddon commented Sep 28, 2023

View reviewed changes

extension/src/plots/model/collect.test.ts Outdated Show resolved Hide resolved

mattseddon commented Sep 28, 2023

View reviewed changes

extension/src/plots/vega/util.ts Outdated Show resolved Hide resolved

mattseddon force-pushed the use-new-templates branch from d2985d1 to a2b254f Compare September 29, 2023 02:57

mattseddon mentioned this pull request Sep 29, 2023

plots: standardise across DVC/Studio/VS Code iterative/dvc#9940

Closed

8 tasks

mattseddon force-pushed the use-new-templates branch from db935bb to e1920fe Compare October 2, 2023 22:07

mattseddon force-pushed the use-new-templates branch 3 times, most recently from 20879a5 to 05fbd09 Compare October 10, 2023 00:21

mattseddon added 8 commits October 16, 2023 10:06

update test fixtures, unit tests and jam new functionality in

c0f2680

use new approach for custom plots (reuse code)

07e17e5

fix zoomable vs zoomed in plot

4f95632

refactor extending vega lite

c260478

add zoom and pan anchor

75cf24d

fill new tooltip anchor

1684130

use new width and height anchors

654db11

switch empty string to empty object from zoom and pan

4a6d460

mattseddon force-pushed the use-new-templates branch from 05fbd09 to 4a6d460 Compare October 15, 2023 23:06

mattseddon mentioned this pull request Oct 19, 2023

Plot tooltip for truncated titles #4858

Merged

mattseddon added 4 commits November 2, 2023 19:12

Merge branch 'main' into use-new-templates

1266f3d

Merge branch 'main' into use-new-templates

e617009

fix svg export issue

d89b179

Merge branch 'main' into use-new-templates

2feebf6

mattseddon commented Nov 26, 2023

View reviewed changes

refactor fill template

c57c33e

codeclimate bot reviewed Nov 27, 2023

View reviewed changes

mattseddon added the do not merge label Nov 27, 2023

mattseddon marked this pull request as ready for review November 27, 2023 01:21

mattseddon requested review from sroy3 and julieg18 as code owners November 27, 2023 01:21

mattseddon mentioned this pull request Nov 27, 2023

plots: standardise across DVC/Studio/VS Code iterative/dvc-render#142

Merged

sroy3 approved these changes Nov 27, 2023

View reviewed changes

julieg18 approved these changes Nov 28, 2023

View reviewed changes

Merge branch 'main' into use-new-templates

38abac9

codeclimate bot reviewed Nov 30, 2023

View reviewed changes

use parent ref to appropriately size plot titles

d59d594

codeclimate bot reviewed Nov 30, 2023

View reviewed changes

mattseddon added 2 commits December 5, 2023 13:33

Merge branch 'main' into use-new-templates

b63f30c

create enum from plot anchors

faa1d34

codeclimate bot reviewed Dec 5, 2023

View reviewed changes

Merge branch 'main' into use-new-templates

fb24a3e

mattseddon mentioned this pull request Dec 5, 2023

Do not mutate plot props when zooming #5066

Closed

codeclimate bot reviewed Dec 5, 2023

View reviewed changes

bump min version to 3.33.0 (breaking change plots diff)

7bde398

codeclimate bot reviewed Dec 6, 2023

View reviewed changes

mattseddon changed the title ~~plots: standardise across DVC/Studio/VS Code~~ Bump min DVC version to 3.33.0 (standardise plots across products) Dec 6, 2023

mattseddon merged commit 5d24dfb into main Dec 6, 2023
8 checks passed

mattseddon deleted the use-new-templates branch December 6, 2023 01:09

mattseddon removed the do not merge label Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump min DVC version to 3.33.0 (standardise plots across products) #4734

Bump min DVC version to 3.33.0 (standardise plots across products) #4734

mattseddon commented Sep 28, 2023 •

edited

Loading

mattseddon Sep 28, 2023

mattseddon Oct 9, 2023

sroy3 Nov 27, 2023

mattseddon Dec 5, 2023

mattseddon Sep 28, 2023

mattseddon Nov 26, 2023

mattseddon Nov 26, 2023

codeclimate bot left a comment

sroy3 Nov 27, 2023

sroy3 Nov 27, 2023

sroy3 Nov 27, 2023

sroy3 Nov 27, 2023

sroy3 Nov 27, 2023

sroy3 Nov 27, 2023

sroy3 Nov 27, 2023

sroy3 Nov 27, 2023

julieg18 left a comment

julieg18 Nov 28, 2023

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot commented Dec 6, 2023

		@@ -1,5 +1,3 @@
		import { Plot } from '../../plots/webview/contract'

		export const MIN_CLI_VERSION = '2.58.1'

		type === 'number' ? 'quantitative' : 'nominal'

		export const createSpec = (

Bump min DVC version to 3.33.0 (standardise plots across products) #4734

Bump min DVC version to 3.33.0 (standardise plots across products) #4734

Conversation

mattseddon commented Sep 28, 2023 • edited Loading

Demo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot commented Dec 6, 2023

mattseddon commented Sep 28, 2023 •

edited

Loading