Code Insights: Improve dashboard select UI (#43029)

* Refactor Popover UI - Add aria attribute to the popover trigger - Fix problem with return focus to target - Remove legacy RAF schedule call in tether - Add new hook API for exposing popover state * Fix Combobox UI - Fix group option layout - Fix scroll into view logic (add scroll into center functionality) * Connect forward ref and truncated text element * Make all dashboard names unique * Refactor DashboardSelect UI (use combobox + popover over the reach ui listbox) * Remove legacy dashboard select UI components * Remove @reach/listbox styles from the main style entry * Fix dashboard content import * Fix combobox popover close logic * Fix dashboard page unit tests * Update dashboard select story * Fix unit tests of UI components that use popover UI * Update feedback prompt unit tests * Fix chart tooltip return to target logic * Fix insight integration tests (dashboard select selectors) * Fix layout and dashboard option selectors * Bring back async tether scheduling * Fix jest snapshots * Remove reach ui listbox package
sourcegraph · Oct 20, 2022 · 05b188c · 05b188c
1 parent 2a7215e
commit 05b188c
Show file tree

Hide file tree

Showing 31 changed files with 601 additions and 609 deletions.
diff --git a/client/web/src/SourcegraphWebApp.scss b/client/web/src/SourcegraphWebApp.scss
@@ -88,7 +88,6 @@ body,
 // Global styles provided by @reach packages. Should be imported once in the global scope.
 @import '@reach/combobox/styles.css';
 @import '@reach/menu-button/styles.css';
-@import '@reach/listbox/styles.css';
 
 // Pages
 @import './api/ApiConsole';

diff --git a/client/web/src/enterprise/insights/components/trancated-text/TruncatedText.tsx b/client/web/src/enterprise/insights/components/trancated-text/TruncatedText.tsx
@@ -9,5 +9,5 @@ import styles from './TruncatedText.module.scss'
 export const TruncatedText = forwardRef((props, reference) => {
     const { as: Component = 'span', className, ...otherProps } = props
 
-    return <Component className={classNames(className, styles.truncatedText)} {...otherProps} />
+    return <Component ref={reference} className={classNames(className, styles.truncatedText)} {...otherProps} />
 }) as ForwardReferenceComponent<'span'>
diff --git a/...src/enterprise/insights/core/backend/gql-backend/methods/get-dashboards/get-dashboards.ts b/...src/enterprise/insights/core/backend/gql-backend/methods/get-dashboards/get-dashboards.ts
@@ -1,4 +1,5 @@
 import { ApolloClient } from '@apollo/client'
+import { groupBy } from 'lodash'
 import { Observable } from 'rxjs'
 import { map } from 'rxjs/operators'
 
@@ -34,7 +35,7 @@ export const getDashboards = (apolloClient: ApolloClient<unknown>, id?: string):
 
             return [
                 ALL_INSIGHTS_DASHBOARD,
-                ...insightsDashboards.nodes.map(
+                ...makeDashboardTitleUnuque(insightsDashboards.nodes).map(
                     (dashboard): InsightDashboard => ({
                         id: dashboard.id,
                         type: InsightsDashboardType.Custom,
@@ -46,7 +47,22 @@ export const getDashboards = (apolloClient: ApolloClient<unknown>, id?: string):
         })
     )
 
-export function deserializeDashboardsOwners(
+function makeDashboardTitleUnuque(dashboards: InsightsDashboardNode[]): InsightsDashboardNode[] {
+    const groupedByTitle = groupBy(dashboards, dashboard => dashboard.title)
+
+    return Object.keys(groupedByTitle).flatMap(title => {
+        if (groupedByTitle[title].length === 1) {
+            return groupedByTitle[title]
+        }
+
+        return groupedByTitle[title].map((dashboard, index) => ({
+            ...dashboard,
+            title: `${dashboard.title} (${index + 1})`,
+        }))
+    })
+}
+
+function deserializeDashboardsOwners(
     dashboardNode: InsightsDashboardNode,
     userNode: InsightsDashboardCurrentUser | null
 ): InsightsDashboardOwner[] {

diff --git a/...eb/src/enterprise/insights/pages/dashboards/dashboard-page/DashboardsContentPage.test.tsx b/...eb/src/enterprise/insights/pages/dashboards/dashboard-page/DashboardsContentPage.test.tsx
@@ -2,7 +2,8 @@ import React from 'react'
 
 import { useApolloClient } from '@apollo/client'
 import { MockedResponse } from '@apollo/client/testing'
-import { waitFor } from '@testing-library/react'
+import { within } from '@testing-library/dom'
+import { act, waitFor } from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
 import sinon from 'sinon'
 
@@ -175,10 +176,13 @@ const triggerDashboardMenuItem = async (
     const dashboardMenu = await waitFor(() => screen.getByRole('img', { name: 'dashboard options' }))
     user.click(dashboardMenu)
 
-    const dashboardMenuItem = screen.getByRole('menuitem', { name: buttonText })
+    const dialog = screen.getByRole('dialog', { hidden: true })
+    const dashboardMenuItem = within(dialog).getByText(buttonText)
 
-    dashboardMenuItem.focus()
-    user.click(dashboardMenuItem)
+    act(() => {
+        dashboardMenuItem.focus()
+        user.click(dashboardMenuItem)
+    })
 }
 
 beforeEach(() => {
@@ -212,8 +216,12 @@ describe('DashboardsContent', () => {
         const chooseDashboard = await waitFor(() => screen.getByRole('button', { name: /Choose a dashboard/ }))
         user.click(chooseDashboard)
 
-        const dashboard2 = screen.getByRole('option', { name: /Global Dashboard 2/ })
-        user.click(dashboard2)
+        const coboboxPopover = screen.getByRole('dialog', { hidden: true })
+        const dashboard2 = coboboxPopover.querySelector('[title="Global Dashboard 2"]')
+
+        if (dashboard2) {
+            user.click(dashboard2)
+        }
 
         expect(history.location.pathname).toEqual('/insights/dashboards/bar')
     })

diff --git a/...s/pages/dashboards/dashboard-page/components/dashboard-select/DashboardSelect.module.scss b/...s/pages/dashboards/dashboard-page/components/dashboard-select/DashboardSelect.module.scss
@@ -1,41 +1,117 @@
-.list {
-    display: block;
-    position: static;
-    width: 100%;
-    overflow: auto;
-    max-height: 32rem;
-    // This value is synced with dashboard select input width
-    max-width: 22.75rem;
-
-    &:focus {
-        // Reach-ui implicitly reset box-shadow to none if list element is focused
-        // to preserve visual state we have to explicitly set a box shadow value to
-        // our standard --dropdown-shadow
-        box-shadow: var(--dropdown-shadow);
+.trigger-button {
+    display: flex;
+    align-items: center;
+    background-color: var(--input-bg);
+    font-weight: normal;
+
+    &--text {
+        display: flex;
+        align-items: baseline;
+        gap: 0.5rem;
+
+        /*
+            Without min-width with value, the flex child containing the other text elements
+            won’t narrow past the “implied width” of those text elements.
+
+            More info (https://css-tricks.com/flexbox-truncated-text/)
+        */
+        min-width: 0;
+        white-space: nowrap;
+        margin-right: 0.25rem;
+    }
+
+    &--icon {
+        flex-shrink: 0;
+        margin-left: auto;
+        // Compensate inner view box SVG padding
+        margin-right: -0.25rem;
+        color: var(--icon-color);
     }
 }
 
-.popover {
-    // Reset reach-ui styles for popover element.
-    outline: none !important;
-    box-shadow: none !important;
-    border: none;
-    background: none;
+.badge {
+    max-width: 8rem;
+    flex-shrink: 0;
 }
 
-.group-label {
-    font-weight: normal;
-    font-size: 0.75rem;
-    border-top: 1px solid var(--border-color);
-    padding: 0.5rem 0.75rem 0;
+.popover {
+    display: flex;
 }
 
-.option {
-    margin: 0.25rem 0;
+.combobox {
+    display: flex;
+    flex-direction: column;
+    height: 100%;
+    max-width: 25rem;
+
+    &-unchanged {
+        [data-user-value] {
+            font-weight: normal;
+        }
+    }
 
-    & + & {
-        margin-top: -0.25rem;
+    &--input-container {
+        padding: 0.5rem;
+        position: sticky;
+        top: 0;
+        z-index: 1;
+        border-bottom: 1px solid var(--border-color);
+        background: var(--dropdown-bg);
+    }
+
+    &--input {
+        font-size: 0.75rem;
+        padding-bottom: 0.25rem;
     }
+
+    &--list {
+        padding-top: 0.25rem;
+        padding-bottom: 0.25rem;
+        @-moz-document url-prefix('') {
+            scrollbar-width: thin;
+            scrollbar-color: var(--text-muted);
+        }
+    }
+
+    &--option-group {
+        [data-group-heading] {
+            position: sticky;
+            top: calc(-0.25rem - 1px);
+            background-color: var(--body-bg);
+        }
+    }
+
+    &--option {
+        display: flex;
+        align-items: baseline;
+        justify-content: space-between;
+        gap: 0.5rem;
+        padding-left: 1rem;
+
+        &[data-highlighted] {
+            /* Badge-secondary variant for improved contrast when background color changes */
+            .badge {
+                background-color: var(--color-bg-1);
+                border-color: var(--color-bg-1);
+            }
+        }
+
+        &-selected {
+            background-color: var(--color-bg-3);
+
+            /* Badge-secondary variant for improved contrast when background color changes */
+            .badge {
+                background-color: var(--color-bg-1);
+                border-color: var(--color-bg-1);
+            }
+        }
+    }
+}
+
+.no-results-found {
+    display: flex;
+    align-items: baseline;
+    padding: 0.25rem 1rem;
 }
 
 .limited-access {
@@ -57,11 +133,3 @@
         font-size: 0.75rem;
     }
 }
-
-.limited-access-wrapper {
-    // Override disabled visual state
-    opacity: 1 !important;
-    margin-bottom: 0;
-    padding-top: 0.5rem;
-    border-top: 1px solid var(--border-color);
-}
diff --git a/...hts/pages/dashboards/dashboard-page/components/dashboard-select/DashboardSelect.story.tsx b/...hts/pages/dashboards/dashboard-page/components/dashboard-select/DashboardSelect.story.tsx
@@ -12,16 +12,16 @@ const decorator: DecoratorFn = story => <WebStory>{() => story()}</WebStory>
 const config: Meta = {
     title: 'web/insights/DashboardSelect',
     decorators: [decorator],
-    parameters: {
-        chromatic: {
-            viewports: [576, 1440],
-        },
-    },
 }
 
 export default config
 
 const DASHBOARDS: InsightDashboard[] = [
+    {
+        type: InsightsDashboardType.Virtual,
+        id: 'ALL_INSIGHTS',
+        title: 'All Insights',
+    },
     {
         type: InsightsDashboardType.Custom,
         id: '101',
@@ -44,26 +44,42 @@ const DASHBOARDS: InsightDashboard[] = [
         type: InsightsDashboardType.Custom,
         id: '104',
         title: 'Sourcegraph',
-        owners: [{ id: '104', title: 'Sourcegraph', type: InsightsDashboardOwnerType.Personal }],
+        owners: [{ id: '104', title: 'Sourcegraph', type: InsightsDashboardOwnerType.Organization }],
     },
     {
         type: InsightsDashboardType.Custom,
         id: '105',
-        title: 'Loooong looo0000ong name of dashboard',
-        owners: [{ id: '104', title: 'Sourcegraph', type: InsightsDashboardOwnerType.Personal }],
+        title: 'Loooong looo0000ong name of dashboard 1',
+        owners: [{ id: '104', title: 'Sourcegraph', type: InsightsDashboardOwnerType.Organization }],
     },
     {
         type: InsightsDashboardType.Custom,
         id: '106',
-        title: 'Loooong looo0000ong name of dashboard',
-        owners: [{ id: '104', title: 'Sourcegraph', type: InsightsDashboardOwnerType.Personal }],
+        title: 'Loooong looo0000ong name of dashboard 2',
+        owners: [{ id: '104', title: 'Sourcegraph', type: InsightsDashboardOwnerType.Organization }],
+    },
+    {
+        type: InsightsDashboardType.Custom,
+        id: '107',
+        title: 'Global dashboard',
+        owners: [{ id: '101', title: 'Personal', type: InsightsDashboardOwnerType.Global }],
+    },
+    {
+        type: InsightsDashboardType.Custom,
+        id: '108',
+        title: 'Global FE dashboard',
+        owners: [{ id: '101', title: 'Personal', type: InsightsDashboardOwnerType.Global }],
     },
 ]
 
 export const DashboardSelectStory: Story = () => {
-    const [value, setValue] = useState<string>()
+    const [dashboard, setDashboard] = useState<InsightDashboard | undefined>()
 
-    return <DashboardSelect value={value} dashboards={DASHBOARDS} onSelect={dashboard => setValue(dashboard.id)} />
+    return (
+        <section style={{ margin: '2rem' }}>
+            <DashboardSelect dashboard={dashboard} dashboards={DASHBOARDS} onSelect={setDashboard} />
+        </section>
+    )
 }
 
 DashboardSelectStory.storyName = 'DashboardSelect'