From d28756f13550e3d1e1e54a2af475710aacadeca7 Mon Sep 17 00:00:00 2001 From: Julie G <43496356+julieg18@users.noreply.github.com> Date: Thu, 14 Sep 2023 09:59:04 -0500 Subject: [PATCH] Add "Add Top-level Plot" command to plots webview (#4664) --- extension/src/pipeline/register.ts | 4 +- extension/src/pipeline/workspace.ts | 4 +- extension/src/plots/webview/messages.ts | 5 ++ extension/src/test/suite/plots/index.test.ts | 18 +++++++ extension/src/webview/contract.ts | 4 ++ webview/src/plots/components/App.test.tsx | 47 ++++++++++++------- .../src/plots/components/PlotsContainer.tsx | 2 +- .../plots/components/emptyState/AddPlots.tsx | 12 ++++- .../plots/components/emptyState/Welcome.tsx | 12 ++++- .../templatePlots/TemplatePlotsWrapper.tsx | 2 + webview/src/plots/util/messages.ts | 6 +++ 11 files changed, 90 insertions(+), 26 deletions(-) diff --git a/extension/src/pipeline/register.ts b/extension/src/pipeline/register.ts index f832be0d78..468db70301 100644 --- a/extension/src/pipeline/register.ts +++ b/extension/src/pipeline/register.ts @@ -1,6 +1,7 @@ import { WorkspacePipeline } from './workspace' import { RegisteredCommands } from '../commands/external' import { InternalCommands } from '../commands/internal' +import { Context, getDvcRootFromContext } from '../vscode/context' export const registerPipelineCommands = ( pipelines: WorkspacePipeline, @@ -13,6 +14,7 @@ export const registerPipelineCommands = ( internalCommands.registerExternalCommand( RegisteredCommands.PIPELINE_ADD_PLOT, - () => pipelines.addTopLevelPlot() + (context: Context) => + pipelines.addTopLevelPlot(getDvcRootFromContext(context)) ) } diff --git a/extension/src/pipeline/workspace.ts b/extension/src/pipeline/workspace.ts index 98039e0b75..6829583d68 100644 --- a/extension/src/pipeline/workspace.ts +++ b/extension/src/pipeline/workspace.ts @@ -63,8 +63,8 @@ export class WorkspacePipeline extends BaseWorkspace { ) } - public async addTopLevelPlot() { - const cwd = await this.getCwd() + public async addTopLevelPlot(overrideRoot?: string) { + const cwd = overrideRoot || (await this.getCwd()) if (!cwd) { return diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index 1031123877..d4b14e99a2 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -76,6 +76,11 @@ export class WebviewMessages { RegisteredCommands.PLOTS_CUSTOM_ADD, this.dvcRoot ) + case MessageFromWebviewType.ADD_PIPELINE_PLOT: + return commands.executeCommand( + RegisteredCommands.PIPELINE_ADD_PLOT, + this.dvcRoot + ) case MessageFromWebviewType.EXPORT_PLOT_DATA_AS_CSV: return this.exportPlotDataAsCsv(message.payload) case MessageFromWebviewType.EXPORT_PLOT_DATA_AS_TSV: diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index b7b3b4fd22..699a4414e6 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -1209,6 +1209,24 @@ suite('Plots Test Suite', () => { ) }) + it('should handle an add pipeline plot message from the webview', async () => { + const { mockMessageReceived } = await buildPlotsWebview({ + disposer: disposable, + plotsDiff: plotsDiffFixture + }) + + const executeCommandSpy = spy(commands, 'executeCommand') + + mockMessageReceived.fire({ + type: MessageFromWebviewType.ADD_PIPELINE_PLOT + }) + + expect(executeCommandSpy).to.be.calledWithExactly( + RegisteredCommands.PIPELINE_ADD_PLOT, + dvcDemoPath + ) + }) + it('should handle the CLI throwing an error', async () => { const { data, errorsModel, mockPlotsDiff, plots, plotsModel } = await buildPlots({ disposer: disposable, plotsDiff: plotsDiffFixture }) diff --git a/extension/src/webview/contract.ts b/extension/src/webview/contract.ts index 4d22b91e5d..ed3708d586 100644 --- a/extension/src/webview/contract.ts +++ b/extension/src/webview/contract.ts @@ -17,6 +17,7 @@ export enum MessageFromWebviewType { APPLY_EXPERIMENT_TO_WORKSPACE = 'apply-experiment-to-workspace', ADD_STARRED_EXPERIMENT_FILTER = 'add-starred-experiment-filter', ADD_CUSTOM_PLOT = 'add-custom-plot', + ADD_PIPELINE_PLOT = 'add-pipeline-plot', CREATE_BRANCH_FROM_EXPERIMENT = 'create-branch-from-experiment', COPY_TO_CLIPBOARD = 'copy-to-clipboard', COPY_STUDIO_LINK = 'copy-studio-link', @@ -110,6 +111,9 @@ export type MessageFromWebview = | { type: MessageFromWebviewType.ADD_CUSTOM_PLOT } + | { + type: MessageFromWebviewType.ADD_PIPELINE_PLOT + } | { type: MessageFromWebviewType.COPY_TO_CLIPBOARD payload: string diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index b96a424f6f..356e3ee8da 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -336,18 +336,18 @@ describe('App', () => { expect(loading).toHaveLength(3) }) - it('should render only get started (buttons: add plots, add experiments, add custom plots) when there are some selected exps, all unselected plots, and no custom plots', async () => { + it('should render only get started (buttons: select plots, add experiments, add custom plots) when there are some selected exps, all unselected plots, and no custom plots', async () => { renderAppWithOptionalData({ hasPlots: true, hasUnselectedPlots: true, selectedRevisions: [{} as Revision] }) const addExperimentsButton = await screen.findByText('Add Experiments') - const addPlotsButton = await screen.findByText('Add Plots') + const selectPlotsButton = await screen.findByText('Select Plots') const addCustomPlotsButton = await screen.findByText('Add Custom Plot') expect(addExperimentsButton).toBeInTheDocument() - expect(addPlotsButton).toBeInTheDocument() + expect(selectPlotsButton).toBeInTheDocument() expect(addCustomPlotsButton).toBeInTheDocument() expect(screen.queryByTestId('section-container')).not.toBeInTheDocument() @@ -361,7 +361,7 @@ describe('App', () => { mockPostMessage.mockReset() - fireEvent.click(addPlotsButton) + fireEvent.click(selectPlotsButton) expect(mockPostMessage).toHaveBeenCalledWith({ type: MessageFromWebviewType.SELECT_PLOTS @@ -427,7 +427,7 @@ describe('App', () => { mockPostMessage.mockReset() }) - it('should render get started (buttons: add plots, add experiments) and custom section when there are some selected exps, all unselected plots, and added custom plots', async () => { + it('should render get started (buttons: select plots, add experiments) and custom section when there are some selected exps, all unselected plots, and added custom plots', async () => { renderAppWithOptionalData({ custom: customPlotsFixture, hasPlots: true, @@ -435,12 +435,12 @@ describe('App', () => { selectedRevisions: [{} as Revision] }) const addExperimentsButton = await screen.findByText('Add Experiments') - const addPlotsButton = await screen.findByText('Add Plots') + const selectPlotsButton = await screen.findByText('Select Plots') const addCustomPlotsButton = screen.queryByText('Add Custom Plot') const customSection = await screen.findByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() - expect(addPlotsButton).toBeInTheDocument() + expect(selectPlotsButton).toBeInTheDocument() expect(addCustomPlotsButton).not.toBeInTheDocument() expect(customSection).toBeInTheDocument() @@ -454,7 +454,7 @@ describe('App', () => { mockPostMessage.mockReset() - fireEvent.click(addPlotsButton) + fireEvent.click(selectPlotsButton) expect(mockPostMessage).toHaveBeenCalledWith({ type: MessageFromWebviewType.SELECT_PLOTS @@ -462,7 +462,7 @@ describe('App', () => { mockPostMessage.mockReset() }) - it('should render only get started (buttons: add experiments, add custom plots) when there are no selected exps and no custom plots', async () => { + it('should render only get started (buttons: add experiments, add custom plots, add plots) when there are no selected exps and no custom plots', async () => { renderAppWithOptionalData({ custom: null, hasPlots: true, @@ -470,13 +470,13 @@ describe('App', () => { selectedRevisions: undefined }) const addExperimentsButton = await screen.findByText('Add Experiments') - const addPlotsButton = screen.queryByText('Add Plots') + const addPlotsButton = await screen.findByText('Add Plot') const addCustomPlotsButton = await screen.findByText('Add Custom Plot') const customSection = screen.queryByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() expect(addCustomPlotsButton).toBeInTheDocument() - expect(addPlotsButton).not.toBeInTheDocument() + expect(addPlotsButton).toBeInTheDocument() expect(customSection).not.toBeInTheDocument() mockPostMessage.mockReset() @@ -492,9 +492,15 @@ describe('App', () => { type: MessageFromWebviewType.ADD_CUSTOM_PLOT }) mockPostMessage.mockReset() + + fireEvent.click(addPlotsButton) + + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.ADD_PIPELINE_PLOT + }) }) - it('should render get started (buttons: add experiments) and custom section when there are no selected exps and added custom plots', async () => { + it('should render get started (buttons: add experiments, add plots) and custom section when there are no selected exps and added custom plots', async () => { renderAppWithOptionalData({ custom: customPlotsFixture, hasPlots: true, @@ -502,13 +508,13 @@ describe('App', () => { selectedRevisions: undefined }) const addExperimentsButton = await screen.findByText('Add Experiments') - const addPlotsButton = screen.queryByText('Add Plots') + const addPlotsButton = await screen.findByText('Add Plot') const addCustomPlotsButton = screen.queryByText('Add Custom Plot') const customSection = await screen.findByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() expect(addCustomPlotsButton).not.toBeInTheDocument() - expect(addPlotsButton).not.toBeInTheDocument() + expect(addPlotsButton).toBeInTheDocument() expect(customSection).toBeInTheDocument() mockPostMessage.mockReset() @@ -517,6 +523,13 @@ describe('App', () => { expect(mockPostMessage).toHaveBeenCalledWith({ type: MessageFromWebviewType.SELECT_EXPERIMENTS }) + + mockPostMessage.mockReset() + + fireEvent.click(addPlotsButton) + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.ADD_PIPELINE_PLOT + }) }) it('should render custom with "No Plots to Display" message when there is no custom plots data', () => { @@ -763,16 +776,14 @@ describe('App', () => { const customSection = screen.getAllByTestId('section-container')[2] - expect( - within(customSection).getByLabelText('Add Plots') - ).toBeInTheDocument() + expect(within(customSection).getByLabelText('Add Plot')).toBeInTheDocument() sendSetDataMessage({ custom: { ...customPlotsFixture, enablePlotCreation: false } }) expect( - within(customSection).queryByLabelText('Add Plots') + within(customSection).queryByLabelText('Add Plot') ).not.toBeInTheDocument() }) diff --git a/webview/src/plots/components/PlotsContainer.tsx b/webview/src/plots/components/PlotsContainer.tsx index 0aa2bf2948..6283eaac24 100644 --- a/webview/src/plots/components/PlotsContainer.tsx +++ b/webview/src/plots/components/PlotsContainer.tsx @@ -74,7 +74,7 @@ export const PlotsContainer: React.FC = ({ menuItems.unshift({ icon: Add, onClick: addPlotsButton.onClick, - tooltip: 'Add Plots' + tooltip: 'Add Plot' }) } diff --git a/webview/src/plots/components/emptyState/AddPlots.tsx b/webview/src/plots/components/emptyState/AddPlots.tsx index 93480a50ba..e162f59f7b 100644 --- a/webview/src/plots/components/emptyState/AddPlots.tsx +++ b/webview/src/plots/components/emptyState/AddPlots.tsx @@ -1,6 +1,7 @@ import React from 'react' import { addCustomPlot, + addPipelinePlot, selectPlots, selectRevisions } from '../../util/messages' @@ -19,12 +20,19 @@ export const AddPlots: React.FC = ({

No Plots to Display

- {hasUnselectedPlots && ( + {hasUnselectedPlots ? ( + ) : ( + )} {!hasCustomPlots && ( diff --git a/webview/src/plots/components/emptyState/Welcome.tsx b/webview/src/plots/components/emptyState/Welcome.tsx index 567e6d95b8..52e8e3aa81 100644 --- a/webview/src/plots/components/emptyState/Welcome.tsx +++ b/webview/src/plots/components/emptyState/Welcome.tsx @@ -1,11 +1,19 @@ import React from 'react' import { StartButton } from '../../../shared/components/button/StartButton' -import { selectRevisions } from '../../util/messages' +import { addPipelinePlot, selectRevisions } from '../../util/messages' export const Welcome: React.FC = () => (

No Plots Detected.

- +
+ + +

Learn how to{' '} diff --git a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx index daf879e6f3..471878fedb 100644 --- a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx +++ b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx @@ -5,6 +5,7 @@ import { TemplatePlots } from './TemplatePlots' import { changeSize } from './templatePlotsSlice' import { PlotsContainer } from '../PlotsContainer' import { PlotsState } from '../../store' +import { addPipelinePlot } from '../../util/messages' export const TemplatePlotsWrapper: React.FC = () => { const { nbItemsPerRow, isCollapsed, height, hasItems } = useSelector( @@ -20,6 +21,7 @@ export const TemplatePlotsWrapper: React.FC = () => { changeSize={changeSize} hasItems={hasItems} height={height} + addPlotsButton={{ onClick: addPipelinePlot }} > diff --git a/webview/src/plots/util/messages.ts b/webview/src/plots/util/messages.ts index a356ad7bea..506903eabc 100644 --- a/webview/src/plots/util/messages.ts +++ b/webview/src/plots/util/messages.ts @@ -8,6 +8,12 @@ export const addCustomPlot = () => type: MessageFromWebviewType.ADD_CUSTOM_PLOT }) +export const addPipelinePlot = () => { + sendMessage({ + type: MessageFromWebviewType.ADD_PIPELINE_PLOT + }) +} + export const exportPlotDataAsCsv = (id: string) => { sendMessage({ payload: id,