From de2d57de2304d8e9727a5331b331e021c5e0d084 Mon Sep 17 00:00:00 2001
From: julieg18 <juliannagal18@gmail.com>
Date: Thu, 21 Dec 2023 18:23:35 -0600
Subject: [PATCH 1/2] Make ribbon block error indicators more obvious

---
 webview/src/plots/components/App.test.tsx     | 25 ++++++++++++++-----
 .../plots/components/ribbon/RevisionIcon.tsx  |  4 +--
 .../plots/components/ribbon/RibbonBlock.tsx   | 15 +++++++----
 .../components/ribbon/RibbonBlockIcon.tsx     | 23 +++++++++++++++++
 .../components/ribbon/styles.module.scss      | 19 +++++++-------
 5 files changed, 62 insertions(+), 24 deletions(-)
 create mode 100644 webview/src/plots/components/ribbon/RibbonBlockIcon.tsx

diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx
index 2544c369ac..526d39c1f1 100644
--- a/webview/src/plots/components/App.test.tsx
+++ b/webview/src/plots/components/App.test.tsx
@@ -49,6 +49,7 @@ import {
   toggleDragAndDropMode as toggleCustomPlotsDragAndDropMode
 } from './customPlots/customPlotsSlice'
 import { comparisonTableInitialState } from './comparisonTable/comparisonTableSlice'
+import plotRibbonStyles from './ribbon/styles.module.scss'
 import { plotsReducers, plotsStore } from '../store'
 import { vsCodeApi } from '../../shared/api'
 import {
@@ -2763,11 +2764,11 @@ describe('App', () => {
       })
     })
 
-    it('should show an error indicator for each revision with an error', () => {
+    it('should show an error icon and highlight the title for each revision with an error', () => {
       renderAppWithOptionalData({
         comparison: comparisonTableFixture,
         selectedRevisions: plotsRevisionsFixture.map(rev => {
-          if (rev.label === 'main') {
+          if (rev.label === 'main' || rev.description === '[exp-e7a67]') {
             return {
               ...rev,
               errors: ['error']
@@ -2776,11 +2777,19 @@ describe('App', () => {
           return rev
         })
       })
-      const errorIndicators = screen.getAllByText('!')
-      expect(errorIndicators).toHaveLength(1)
+      const mainTitle = within(screen.getByTestId('ribbon-main')).getByText(
+        'main'
+      )
+      const expTitle = within(screen.getByTestId('ribbon-exp-e7a67')).getByText(
+        '[exp-e7a67]'
+      )
+      const errorIndicators = screen.getAllByLabelText('Error Icon')
+      expect(errorIndicators).toHaveLength(2)
+      expect(mainTitle).toHaveClass(plotRibbonStyles.errorIndicator)
+      expect(expTitle).toHaveClass(plotRibbonStyles.errorIndicator)
     })
 
-    it('should not show an error indicator for a loading revision', () => {
+    it('should not show an error icon or highlight the title for a loading revision', () => {
       renderAppWithOptionalData({
         comparison: comparisonTableFixture,
         selectedRevisions: plotsRevisionsFixture.map(rev => {
@@ -2794,7 +2803,11 @@ describe('App', () => {
           return rev
         })
       })
-      expect(screen.queryByText('!')).not.toBeInTheDocument()
+      expect(screen.queryByLabelText('Error Icon')).not.toBeInTheDocument()
+      const titleText = within(screen.getByTestId('ribbon-main')).getByText(
+        'main'
+      )
+      expect(titleText).not.toHaveClass(plotRibbonStyles.errorIndicator)
     })
   })
 
diff --git a/webview/src/plots/components/ribbon/RevisionIcon.tsx b/webview/src/plots/components/ribbon/RevisionIcon.tsx
index 21770886d5..51930b47d7 100644
--- a/webview/src/plots/components/ribbon/RevisionIcon.tsx
+++ b/webview/src/plots/components/ribbon/RevisionIcon.tsx
@@ -5,10 +5,8 @@ import styles from './styles.module.scss'
 
 export const RevisionIcon: React.FC<{
   fetched: boolean
-  errors?: string[]
-}> = ({ fetched, errors }) => (
+}> = ({ fetched }) => (
   <div className={styles.iconPlaceholder}>
-    {fetched && errors && '!'}
     {!fetched && (
       <VSCodeProgressRing className={cx(styles.fetching, 'chromatic-ignore')} />
     )}
diff --git a/webview/src/plots/components/ribbon/RibbonBlock.tsx b/webview/src/plots/components/ribbon/RibbonBlock.tsx
index 2a62de70cb..5242995ae6 100644
--- a/webview/src/plots/components/ribbon/RibbonBlock.tsx
+++ b/webview/src/plots/components/ribbon/RibbonBlock.tsx
@@ -1,12 +1,14 @@
 import { Revision } from 'dvc/src/plots/webview/contract'
+import cx from 'classnames'
 import React from 'react'
 import styles from './styles.module.scss'
+import { RibbonBlockIcon } from './RibbonBlockIcon'
 import { RibbonBlockTooltip } from './RibbonBlockTooltip'
 import { RevisionIcon } from './RevisionIcon'
 import { Icon } from '../../../shared/components/Icon'
 import Tooltip from '../../../shared/components/tooltip/Tooltip'
 import { CopyButton } from '../../../shared/components/copyButton/CopyButton'
-import { Close, Info } from '../../../shared/components/icons'
+import { Close } from '../../../shared/components/icons'
 
 interface RibbonBlockProps {
   revision: Revision
@@ -27,6 +29,7 @@ export const RibbonBlock: React.FC<RibbonBlockProps> = ({
     id,
     label
   } = revision
+  const hasError = fetched && !!errors
 
   const mainContent = (
     <li
@@ -34,12 +37,14 @@ export const RibbonBlock: React.FC<RibbonBlockProps> = ({
       style={{ borderColor: displayColor }}
       data-testid={`ribbon-${id}`}
     >
-      <Info width={14} height={14} className={styles.infoIcon} />
+      <RibbonBlockIcon hasError={hasError} />
       <div className={styles.label}>
         {description ? (
           <>
             <div className={styles.subtitle}>{label}</div>
-            <div className={styles.title}>
+            <div
+              className={cx(styles.title, hasError && styles.errorIndicator)}
+            >
               {description}
               <CopyButton
                 value={description.replace(/[[\]]/g, '')}
@@ -48,14 +53,14 @@ export const RibbonBlock: React.FC<RibbonBlockProps> = ({
             </div>
           </>
         ) : (
-          <div className={styles.title}>
+          <div className={cx(styles.title, hasError && styles.errorIndicator)}>
             {label}
             <CopyButton value={label} className={styles.copyButton} />
           </div>
         )}
       </div>
       <div className={styles.iconPlaceholder}>
-        <RevisionIcon errors={errors} fetched={fetched} />
+        <RevisionIcon fetched={fetched} />
       </div>
       <Tooltip content="Clear" placement="bottom" delay={500}>
         <button className={styles.clearButton} onClick={onClear}>
diff --git a/webview/src/plots/components/ribbon/RibbonBlockIcon.tsx b/webview/src/plots/components/ribbon/RibbonBlockIcon.tsx
new file mode 100644
index 0000000000..5df2ce3549
--- /dev/null
+++ b/webview/src/plots/components/ribbon/RibbonBlockIcon.tsx
@@ -0,0 +1,23 @@
+import React from 'react'
+import cx from 'classnames'
+import styles from './styles.module.scss'
+import { Error, Info } from '../../../shared/components/icons'
+
+export const RibbonBlockIcon: React.FC<{
+  hasError: boolean
+}> = ({ hasError }) =>
+  hasError ? (
+    <Error
+      width={14}
+      height={14}
+      className={cx(styles.blockIcon, styles.error)}
+      aria-label="Error Icon"
+    />
+  ) : (
+    <Info
+      width={14}
+      height={14}
+      className={styles.blockIcon}
+      aria-label="Info Icon"
+    />
+  )
diff --git a/webview/src/plots/components/ribbon/styles.module.scss b/webview/src/plots/components/ribbon/styles.module.scss
index 164ba5d871..fc081868ea 100644
--- a/webview/src/plots/components/ribbon/styles.module.scss
+++ b/webview/src/plots/components/ribbon/styles.module.scss
@@ -1,11 +1,16 @@
 @import '../../../shared/variables';
 @import '../../../shared/mixins';
 
+.errorIndicator {
+  color: $error-color;
+}
+
 .copyButton {
   display: none;
   position: absolute;
   right: 0;
   top: 0;
+  fill: $fg-color;
 }
 
 .tooltipColumn {
@@ -96,11 +101,6 @@
   align-items: center;
   font-size: 0.8125rem;
 
-  svg {
-    fill: $fg-color;
-    display: block;
-  }
-
   &:hover .copyButton {
     display: inline;
     font-size: 0.8125rem;
@@ -110,18 +110,16 @@
     margin-left: -4px;
     height: 18px;
     width: 18px;
-    color: $error-color;
-    text-align: center;
   }
 
   .fetching {
     height: 18px;
     width: 18px;
   }
+}
 
-  .infoIcon {
-    margin-left: 5px;
-  }
+.blockIcon {
+  margin-left: 5px;
 }
 
 .label {
@@ -153,6 +151,7 @@
 
 .clearButton {
   background-color: transparent;
+  color: $fg-color;
   border: none;
   padding: 3px;
   opacity: 0;

From 9f5356aefbb57e12dafdc9dafb73dc274a373976 Mon Sep 17 00:00:00 2001
From: julieg18 <juliannagal18@gmail.com>
Date: Fri, 22 Dec 2023 09:29:37 -0600
Subject: [PATCH 2/2] cleanup

---
 .../components/ribbon/{RevisionIcon.tsx => LoadingIcon.tsx} | 2 +-
 webview/src/plots/components/ribbon/RibbonBlock.tsx         | 6 ++----
 webview/src/plots/components/ribbon/RibbonBlockIcon.tsx     | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)
 rename webview/src/plots/components/ribbon/{RevisionIcon.tsx => LoadingIcon.tsx} (90%)

diff --git a/webview/src/plots/components/ribbon/RevisionIcon.tsx b/webview/src/plots/components/ribbon/LoadingIcon.tsx
similarity index 90%
rename from webview/src/plots/components/ribbon/RevisionIcon.tsx
rename to webview/src/plots/components/ribbon/LoadingIcon.tsx
index 51930b47d7..1bdff2f47e 100644
--- a/webview/src/plots/components/ribbon/RevisionIcon.tsx
+++ b/webview/src/plots/components/ribbon/LoadingIcon.tsx
@@ -3,7 +3,7 @@ import cx from 'classnames'
 import { VSCodeProgressRing } from '@vscode/webview-ui-toolkit/react'
 import styles from './styles.module.scss'
 
-export const RevisionIcon: React.FC<{
+export const LoadingIcon: React.FC<{
   fetched: boolean
 }> = ({ fetched }) => (
   <div className={styles.iconPlaceholder}>
diff --git a/webview/src/plots/components/ribbon/RibbonBlock.tsx b/webview/src/plots/components/ribbon/RibbonBlock.tsx
index 5242995ae6..1cb4ad929b 100644
--- a/webview/src/plots/components/ribbon/RibbonBlock.tsx
+++ b/webview/src/plots/components/ribbon/RibbonBlock.tsx
@@ -4,7 +4,7 @@ import React from 'react'
 import styles from './styles.module.scss'
 import { RibbonBlockIcon } from './RibbonBlockIcon'
 import { RibbonBlockTooltip } from './RibbonBlockTooltip'
-import { RevisionIcon } from './RevisionIcon'
+import { LoadingIcon } from './LoadingIcon'
 import { Icon } from '../../../shared/components/Icon'
 import Tooltip from '../../../shared/components/tooltip/Tooltip'
 import { CopyButton } from '../../../shared/components/copyButton/CopyButton'
@@ -59,9 +59,7 @@ export const RibbonBlock: React.FC<RibbonBlockProps> = ({
           </div>
         )}
       </div>
-      <div className={styles.iconPlaceholder}>
-        <RevisionIcon fetched={fetched} />
-      </div>
+      <LoadingIcon fetched={fetched} />
       <Tooltip content="Clear" placement="bottom" delay={500}>
         <button className={styles.clearButton} onClick={onClear}>
           <Icon icon={Close} width={12} height={12} />
diff --git a/webview/src/plots/components/ribbon/RibbonBlockIcon.tsx b/webview/src/plots/components/ribbon/RibbonBlockIcon.tsx
index 5df2ce3549..7a8c1c76e3 100644
--- a/webview/src/plots/components/ribbon/RibbonBlockIcon.tsx
+++ b/webview/src/plots/components/ribbon/RibbonBlockIcon.tsx
@@ -10,7 +10,7 @@ export const RibbonBlockIcon: React.FC<{
     <Error
       width={14}
       height={14}
-      className={cx(styles.blockIcon, styles.error)}
+      className={cx(styles.blockIcon, styles.errorIndicator)}
       aria-label="Error Icon"
     />
   ) : (