Merge create plot commands #4680

julieg18 · 2023-09-18T14:50:22Z

combine "Add Custom Plot" and "Add Plot" command into one. This adds an explanation for the "file selector" that appears when you add a top-level plot and simplifies some logic.

Demo

~~https://github.com/iterative/vscode-dvc/assets/43496356/62dad7cc-df0d-4051-a79e-981fe02f68fe~~

~~https://github.com/iterative/vscode-dvc/assets/43496356/5e50305c-c30a-4406-a27f-5c5af7e6f922~~

Screen.Recording.2023-09-20.at.7.21.17.AM.mov

To Do

Rename "top-level" to "Data Series"

Part of #4654

julieg18 · 2023-09-18T16:06:19Z

extension/src/commands/util.ts

Is there a better place for these two commands that are registered in extension.ts? I couldn't think of a place that didn't feel confusing 🤔

julieg18 · 2023-09-18T16:08:42Z

webview/src/plots/components/ribbon/Ribbon.tsx

@@ -75,6 +76,14 @@ export const Ribbon: React.FC = () => {
          text={`${revisions.length} of ${MAX_NB_EXP}`}
        />
      </li>
+      <li className={styles.buttonWrapper}>


Example with primary colors:

As per the guidelines there should only be one primary button. I think Having Add Plot as the primary button now makes more sense than the revision selector but up to you.

julieg18 · 2023-09-18T16:10:38Z

webview/src/plots/util/messages.ts

  })

-export const addPipelinePlot = () => {
+export const addCustomPlot = () =>


I chose to keep the add plot button in the custom section since we still have the trash icon there, so I thought it made sense to have the add button next to it.

But we can remove it if preferred.

If you think the UX is better then it is ok to leave and we can even keep the add plot button above the Data Series section but do keep in mind that we will to maintain this code. Any code that runs can eventually cause bugs.

Are we still keeping this?

I thought we agreed to keep it for now, but it wouldn't be the first time I misunderstood what was said in a meeting 😊. I'll go ahead and take off the + button. The addCustomPlot util would stay since we use it in the error screen.

Actually, after testing it, I think we should keep it for now until we add a button to our "No Plots Added" screen. I was going to do that in a separate pr since it's only partially related to this pr plus this one is getting on the larger side at this point.

mattseddon · 2023-09-18T22:33:08Z

webview/src/plots/components/ribbon/Ribbon.tsx

@@ -75,6 +76,14 @@ export const Ribbon: React.FC = () => {
          text={`${revisions.length} of ${MAX_NB_EXP}`}
        />
      </li>
+      <li className={styles.buttonWrapper}>


As per the guidelines there should only be one primary button. I think Having Add Plot as the primary button now makes more sense than the revision selector but up to you.

mattseddon · 2023-09-18T22:38:36Z

webview/src/plots/util/messages.ts

  })

-export const addPipelinePlot = () => {
+export const addCustomPlot = () =>


If you think the UX is better then it is ok to leave and we can even keep the add plot button above the Data Series section but do keep in mind that we will to maintain this code. Any code that runs can eventually cause bugs.

extension/src/commands/util.ts

mattseddon · 2023-09-18T22:53:23Z

extension/src/commands/util.ts

+  TOP_LEVEL = 'top-level'
+}
+
+export const addPlotCommand = async (context: Context) => {


[I] These should go in a new plots/quickPick file. Can you also please check to see if we will get two analytics events each time this is called (I don't think we want that).

Can you also please check to see if we will get two analytics events each time this is called (I don't think we want that).

Oh, I think we would get two since addPlot fires one and then "add custom"/"add pipeline" would fire another. We could remove the "add custom" and "add pipeline" commands and add a type argument to the "add plot" command 🤔

extension/src/extension.ts

julieg18 · 2023-09-19T15:47:07Z

Resolved comments and re-requesting reviews since quite a bit of the solution has been updated! cc @mattseddon

codeclimate · 2023-09-20T12:30:42Z

webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx

-    enablePlotCreation,
-    hasAddedPlots
-  } = useSelector((state: PlotsState) => state.custom)
+  const { plotsIds, nbItemsPerRow, isCollapsed, height, hasAddedPlots } =


Similar blocks of code found in 3 locations. Consider refactoring.

This reverts commit ec3c5ed.

codeclimate · 2023-09-20T12:39:28Z

Code Climate has analyzed commit 0aeb1cc and detected 0 issues on this pull request.

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

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

View more on Code Climate.

Merge create plot commands

b2f6d02

julieg18 added the product PR that affects product label Sep 18, 2023

julieg18 self-assigned this Sep 18, 2023

Add tests

d7d3f5f

julieg18 commented Sep 18, 2023

View reviewed changes

Fix e2e tests

955edf5

julieg18 marked this pull request as ready for review September 18, 2023 16:53

julieg18 requested review from mattseddon and sroy3 as code owners September 18, 2023 16:53

julieg18 requested a review from shcheklein September 18, 2023 16:55

mattseddon approved these changes Sep 18, 2023

View reviewed changes

julieg18 added 3 commits September 19, 2023 10:01

resolve comments

9311223

fix typo

68ff206

Merge branch 'main' into merge-create-plot-commands

403e93f

julieg18 requested a review from mattseddon September 19, 2023 15:47

Rename "Top-Level" to "Data Series"

6233c89

mattseddon approved these changes Sep 20, 2023

View reviewed changes

julieg18 and others added 2 commits September 20, 2023 07:18

Remove + button in custom plots section

ec3c5ed

Merge branch 'main' into merge-create-plot-commands

3329859

codeclimate bot reviewed Sep 20, 2023

View reviewed changes

Revert "Remove + button in custom plots section"

0aeb1cc

This reverts commit ec3c5ed.

julieg18 enabled auto-merge (squash) September 20, 2023 12:42

julieg18 merged commit f6654fd into main Sep 20, 2023
3 checks passed

julieg18 deleted the merge-create-plot-commands branch September 20, 2023 12:45

julieg18 mentioned this pull request Sep 20, 2023

Add action buttons to empty plot sections #4694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge create plot commands #4680

Merge create plot commands #4680

julieg18 commented Sep 18, 2023 •

edited

Loading

julieg18 Sep 18, 2023

julieg18 Sep 18, 2023

mattseddon Sep 18, 2023

julieg18 Sep 18, 2023

mattseddon Sep 18, 2023

mattseddon Sep 20, 2023

julieg18 Sep 20, 2023

julieg18 Sep 20, 2023 •

edited

Loading

mattseddon Sep 18, 2023

mattseddon Sep 18, 2023

mattseddon Sep 18, 2023

julieg18 Sep 19, 2023

julieg18 commented Sep 19, 2023

codeclimate bot Sep 20, 2023

codeclimate bot commented Sep 20, 2023

Merge create plot commands #4680

Merge create plot commands #4680

Conversation

julieg18 commented Sep 18, 2023 • edited Loading

Demo

To Do

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 Sep 20, 2023 • edited Loading

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 commented Sep 19, 2023

codeclimate bot Sep 20, 2023

Choose a reason for hiding this comment

codeclimate bot commented Sep 20, 2023

julieg18 commented Sep 18, 2023 •

edited

Loading

julieg18 Sep 20, 2023 •

edited

Loading