[UI] Make visualization tab easier to understand (#3717)

* Rename artifacts tab to visualizations and add documentation link * Show a banner when no visualizations * Clean up code * Update snapshots * Fix banner tests * Add unit test for visualization creator * Update VisualizationCreator.tsx * Update VisualizationCreator.tsx
kubeflow · May 9, 2020 · b1c9976 · b1c9976
1 parent d2c784a
commit b1c9976
Show file tree

Hide file tree

Showing 11 changed files with 176 additions and 302 deletions.
diff --git a/frontend/src/components/Banner.test.tsx b/frontend/src/components/Banner.test.tsx
@@ -52,6 +52,34 @@ describe('Banner', () => {
     expect(tree).toMatchSnapshot();
   });
 
+  it('shows troubleshooting link instructed by prop', () => {
+    const tree = shallow(<Banner message='Some message' showTroubleshootingGuideLink={true} />);
+    expect(tree).toMatchInlineSnapshot(`
+      <div
+        className="flex banner mode"
+      >
+        <div
+          className="message"
+        >
+          <pure(ErrorIcon)
+            className="icon"
+          />
+          Some message
+        </div>
+        <div
+          className="flex"
+        >
+          <a
+            className="troubleShootingLink"
+            href="https://www.kubeflow.org/docs/pipelines/troubleshooting"
+          >
+            Troubleshooting guide
+          </a>
+        </div>
+      </div>
+    `);
+  });
+
   it('opens details dialog when button is clicked', () => {
     const tree = shallow(<Banner message='hello' additionalInfo='world' />);
     tree

diff --git a/frontend/src/components/Banner.tsx b/frontend/src/components/Banner.tsx
@@ -69,6 +69,7 @@ export interface BannerProps {
   additionalInfo?: string;
   message?: string;
   mode?: Mode;
+  showTroubleshootingGuideLink?: boolean;
   refresh?: () => void;
 }
 
@@ -120,12 +121,14 @@ class Banner extends React.Component<BannerProps, BannerState> {
           {this.props.message}
         </div>
         <div className={commonCss.flex}>
-          <a
-            className={css.troubleShootingLink}
-            href='https://www.kubeflow.org/docs/pipelines/troubleshooting'
-          >
-            Troubleshooting guide
-          </a>
+          {this.props.showTroubleshootingGuideLink && (
+            <a
+              className={css.troubleShootingLink}
+              href='https://www.kubeflow.org/docs/pipelines/troubleshooting'
+            >
+              Troubleshooting guide
+            </a>
+          )}
           {this.props.additionalInfo && (
             <Button
               className={classes(css.button, css.detailsButton)}

diff --git a/frontend/src/components/Router.tsx b/frontend/src/components/Router.tsx
@@ -258,6 +258,7 @@ class RoutedPage extends React.Component<{ route?: RouteConfig }, RouteComponent
             mode={this.state.bannerProps.mode}
             additionalInfo={this.state.bannerProps.additionalInfo}
             refresh={this.state.bannerProps.refresh}
+            showTroubleshootingGuideLink={true}
           />
         )}
         <Switch>

diff --git a/frontend/src/components/__snapshots__/Banner.test.tsx.snap b/frontend/src/components/__snapshots__/Banner.test.tsx.snap
@@ -14,14 +14,7 @@ exports[`Banner defaults to error mode 1`] = `
   </div>
   <div
     className="flex"
-  >
-    <a
-      className="troubleShootingLink"
-      href="https://www.kubeflow.org/docs/pipelines/troubleshooting"
-    >
-      Troubleshooting guide
-    </a>
-  </div>
+  />
 </div>
 `;
 
@@ -40,12 +33,6 @@ exports[`Banner shows "Details" button and has dialog when there is additional i
   <div
     className="flex"
   >
-    <a
-      className="troubleShootingLink"
-      href="https://www.kubeflow.org/docs/pipelines/troubleshooting"
-    >
-      Troubleshooting guide
-    </a>
     <WithStyles(Button)
       className="button detailsButton"
       onClick={[Function]}
@@ -92,12 +79,6 @@ exports[`Banner shows "Refresh" button when passed a refresh function 1`] = `
   <div
     className="flex"
   >
-    <a
-      className="troubleShootingLink"
-      href="https://www.kubeflow.org/docs/pipelines/troubleshooting"
-    >
-      Troubleshooting guide
-    </a>
     <WithStyles(Button)
       className="button refreshButton"
       onClick={[Function]}
@@ -122,14 +103,7 @@ exports[`Banner uses error mode when instructed 1`] = `
   </div>
   <div
     className="flex"
-  >
-    <a
-      className="troubleShootingLink"
-      href="https://www.kubeflow.org/docs/pipelines/troubleshooting"
-    >
-      Troubleshooting guide
-    </a>
-  </div>
+  />
 </div>
 `;
 
@@ -147,13 +121,6 @@ exports[`Banner uses warning mode when instructed 1`] = `
   </div>
   <div
     className="flex"
-  >
-    <a
-      className="troubleShootingLink"
-      href="https://www.kubeflow.org/docs/pipelines/troubleshooting"
-    >
-      Troubleshooting guide
-    </a>
-  </div>
+  />
 </div>
 `;
diff --git a/frontend/src/components/viewers/VisualizationCreator.test.tsx b/frontend/src/components/viewers/VisualizationCreator.test.tsx
@@ -16,9 +16,11 @@
 
 import * as React from 'react';
 import { shallow, mount } from 'enzyme';
+import { render, screen, fireEvent } from '@testing-library/react';
 import { PlotType } from './Viewer';
 import VisualizationCreator, { VisualizationCreatorConfig } from './VisualizationCreator';
 import { ApiVisualizationType } from '../../apis/visualization';
+import { diffHTML } from 'src/TestUtils';
 
 describe('VisualizationCreator', () => {
   it('does not render component when no config is provided', () => {
@@ -331,4 +333,50 @@ describe('VisualizationCreator', () => {
   it('returns friendly display name', () => {
     expect(VisualizationCreator.prototype.getDisplayName()).toBe('Visualization Creator');
   });
+
+  it('can be configured as collapsed initially and clicks to open', () => {
+    const baseConfig: VisualizationCreatorConfig = {
+      isBusy: false,
+      onGenerate: jest.fn(),
+      type: PlotType.VISUALIZATION_CREATOR,
+      collapsedInitially: false,
+    };
+    const { container: baseContainer } = render(<VisualizationCreator configs={[baseConfig]} />);
+    const { container } = render(
+      <VisualizationCreator
+        configs={[
+          {
+            ...baseConfig,
+            collapsedInitially: true,
+          },
+        ]}
+      />,
+    );
+    expect(container).toMatchInlineSnapshot(`
+      <div>
+        <button
+          class="MuiButtonBase-root-114 MuiButton-root-88 MuiButton-text-90 MuiButton-flat-93"
+          tabindex="0"
+          type="button"
+        >
+          <span
+            class="MuiButton-label-89"
+          >
+            create visualizations manually
+          </span>
+          <span
+            class="MuiTouchRipple-root-117"
+          />
+        </button>
+      </div>
+    `);
+    const button = screen.getByText('create visualizations manually');
+    fireEvent.click(button);
+    // expanding a visualization creator is equivalent to rendering a non-collapsed visualization creator
+    expect(diffHTML({ base: baseContainer.innerHTML, update: container.innerHTML }))
+      .toMatchInlineSnapshot(`
+      Snapshot Diff:
+      Compared values have no visual difference.
+    `);
+  });
 });
diff --git a/frontend/src/components/viewers/VisualizationCreator.tsx b/frontend/src/components/viewers/VisualizationCreator.tsx
@@ -29,13 +29,16 @@ import 'brace/ext/language_tools';
 import 'brace/mode/json';
 import 'brace/mode/python';
 import 'brace/theme/github';
+import Button from '@material-ui/core/Button';
 
 export interface VisualizationCreatorConfig extends ViewerConfig {
   allowCustomVisualizations?: boolean;
   // Whether there is currently a visualization being generated or not.
   isBusy?: boolean;
   // Function called to generate a visualization.
   onGenerate?: (visualizationArguments: string, source: string, type: ApiVisualizationType) => void;
+  // Facilitate testing by not collapsing by default.
+  collapsedInitially?: boolean;
 }
 
 interface VisualizationCreatorProps {
@@ -44,6 +47,7 @@ interface VisualizationCreatorProps {
 }
 
 interface VisualizationCreatorState {
+  expanded: boolean;
   // arguments is expected to be a JSON object in string form.
   arguments: string;
   code: string;
@@ -52,14 +56,12 @@ interface VisualizationCreatorState {
 }
 
 class VisualizationCreator extends Viewer<VisualizationCreatorProps, VisualizationCreatorState> {
-  constructor(props: VisualizationCreatorProps) {
-    super(props);
-    this.state = {
-      arguments: '',
-      code: '',
-      source: '',
-    };
-  }
+  public state: VisualizationCreatorState = {
+    expanded: !this.props.configs[0]?.collapsedInitially,
+    arguments: '',
+    code: '',
+    source: '',
+  };
 
   public getDisplayName(): string {
     return 'Visualization Creator';
@@ -87,6 +89,14 @@ class VisualizationCreator extends Viewer<VisualizationCreatorProps, Visualizati
 
     const argumentsPlaceholder = this.getArgumentPlaceholderForType(selectedType);
 
+    if (!this.state.expanded) {
+      return (
+        <Button variant='text' onClick={this.handleExpansion}>
+          create visualizations manually
+        </Button>
+      );
+    }
+
     return (
       <div
         style={{
@@ -258,6 +268,12 @@ class VisualizationCreator extends Viewer<VisualizationCreatorProps, Visualizati
         .replace(new RegExp('\t', 'g'), '&nbsp&nbsp&nbsp&nbsp')
     );
   }
+
+  private handleExpansion = () => {
+    this.setState({
+      expanded: true,
+    });
+  };
 }
 
 export default VisualizationCreator;
diff --git a/frontend/src/lib/OutputArtifactLoader.ts b/frontend/src/lib/OutputArtifactLoader.ts
@@ -18,19 +18,14 @@ import {
   Api,
   Artifact,
   ArtifactType,
-  Context,
   Event,
   Execution,
   GetArtifactsByIDRequest,
   GetArtifactsByIDResponse,
   GetArtifactTypesRequest,
   GetArtifactTypesResponse,
-  GetContextByTypeAndNameRequest,
-  GetContextByTypeAndNameResponse,
   GetEventsByExecutionIDsRequest,
   GetEventsByExecutionIDsResponse,
-  GetExecutionsByContextRequest,
-  GetExecutionsByContextResponse,
 } from '@kubeflow/frontend';
 import { csvParseRows } from 'd3-dsv';
 import { ApiVisualization, ApiVisualizationType } from '../apis/visualization';
@@ -407,82 +402,6 @@ export class OutputArtifactLoader {
   }
 }
 
-/**
- * @throws error when network error
- * @returns context, returns undefined when context with the pod name not found
- */
-async function getMlmdContext(argoPodName: string): Promise<Context | undefined> {
-  if (argoPodName.split('-').length < 3) {
-    throw new Error('argoPodName has fewer than 3 parts');
-  }
-
-  // argoPodName has the general form "pipelineName-workflowId-executionId".
-  // All components of a pipeline within a single run will have the same
-  // "pipelineName-workflowId" prefix.
-  const pipelineName = argoPodName
-    .split('-')
-    .slice(0, -2)
-    .join('_');
-  const runID = argoPodName
-    .split('-')
-    .slice(0, -1)
-    .join('-');
-  const contextName = pipelineName + '.' + runID;
-
-  const request = new GetContextByTypeAndNameRequest();
-  request.setTypeName('run');
-  request.setContextName(contextName);
-  let res: GetContextByTypeAndNameResponse;
-  try {
-    res = await Api.getInstance().metadataStoreService.getContextByTypeAndName(request);
-  } catch (err) {
-    err.message = 'Failed to getContextsByTypeAndName: ' + err.message;
-    throw err;
-  }
-
-  return res.getContext();
-}
-
-/**
- * @throws error when network error
- * @returns execution, returns undefined when not found or not yet complete
- */
-async function getExecutionInContextWithPodName(
-  argoPodName: string,
-  context: Context,
-): Promise<Execution | undefined> {
-  const contextId = context.getId();
-  if (!contextId) {
-    throw new Error('Context must have an ID');
-  }
-
-  const request = new GetExecutionsByContextRequest();
-  request.setContextId(contextId);
-  let res: GetExecutionsByContextResponse;
-  try {
-    res = await Api.getInstance().metadataStoreService.getExecutionsByContext(request);
-  } catch (err) {
-    err.message = 'Failed to getExecutionsByContext: ' + err.message;
-    throw err;
-  }
-
-  const executionList = res.getExecutionsList();
-  const foundExecution = executionList.find(execution => {
-    const executionPodName = execution.getPropertiesMap().get('kfp_pod_name');
-    return executionPodName && executionPodName.getStringValue() === argoPodName;
-  });
-  if (!foundExecution) {
-    return undefined; // Not found, this is expected to happen normally when there's no mlmd data.
-  }
-  const state = foundExecution.getPropertiesMap().get('state');
-  // Both complete and cached executions are considered valid.
-  if (state && ['complete', 'cached'].includes(state.getStringValue())) {
-    return foundExecution;
-  }
-  // No valid execution found.
-  return undefined;
-}
-
 /**
  * @throws error when network error or invalid data
  */

diff --git a/frontend/src/pages/ArchivedExperiments.tsx b/frontend/src/pages/ArchivedExperiments.tsx
@@ -33,10 +33,6 @@ interface ArchivedExperimentsState {}
 export class ArchivedExperiments extends Page<ArchivedExperimentsProp, ArchivedExperimentsState> {
   private _experimentlistRef = React.createRef<ExperimentList>();
 
-  constructor(props: any) {
-    super(props);
-  }
-
   public getInitialToolbarState(): ToolbarProps {
     const buttons = new Buttons(this.props, this.refresh.bind(this));
     return {