Code Insights: Improve dashboard select UI #43029

vovakulikov · 2022-10-17T02:26:29Z

Problem

This PR improves dashboard select UI on the code insight dashboard page. Previously this select was built with Listbox UI from reach-ui. And it was good enough but we had a few problems with

Keyboard navigation since listbox doesn't work well with anything that is not listbox component (it's bad because we want to have search filter input within listbox popover)
Listbox options accessibility. Keyboard navigation doesn't support scroll into view functionality for focused/active listbox options.
Listbox Popover has to be fixed size and doesn't have smart position logic that we have with out in-house popover component
Filter highlighting logic, we want to highlight parts that match with filter value in the input and we had to write this logic ourselves back then.

Solution

In this PR we replace Listbox UI with combination of wildcard Popover and Combobox UI. This gives us better UI/UX.

Better keyboard navigation. All options are accessible from keyboard. It works nicely with search filter input (it has autocompletion as you navigate through options)
Better Popover logic. We use our in-house popover this means that select UI popover will fit in the visible viewport area based on content and available area below the select element.
Slightly updated UI. Dashboard select UI is built now with more or less the same layout concept as all other comboboxes in the app. (Like SearchContextPicker on the home page)

Tradeoffs

Since Combobox UI works only with unique strings we can't have dashboard with the same names in the options list. Because of this this PR enforces unique dashboard names by adding (1), (2), ... (N) suffix (example: Insights dashboard, Insights dashboard become Insights dashboard (1) and Insights dashboard (2))
If you have selected dashboard and then you open the dashboard select popover you will see that search input has its name as a value. This is done for a better keyboard navigation (TL;DR it's reach-ui problem). This is the only way to enforce that keyboard navigation starts with selected element and not from the very beginning of the options list. More details in the reach-ui issues about it Combobox: Make it possible to set initial navigationValue reach/reach-ui#979

Test plan

Make sure that you can pick any dashboard that you can see in the dashboard select UI
Check that all dashboard options are accessible from keyboard
Check that dashboard select popover is placed in the right position in different screen sizes.
Check that long dashboard names are handled properly (truncated) in select trigger button and in combobox options

App preview:

Web Direct link: https://sg-web-vk-improve-dashboard-select.onrender.com/search
Storybook

Check out the client app preview documentation to learn more.

sg-e2e-regression-test-bob · 2022-10-17T02:33:11Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
-0.08% (-2.25 kb)	-0.16% (-24.23 kb) 🔽	-0.19% (-21.97 kb) 🔽	-0.41% (-3) 🔽

Look at the Statoscope report for a full comparison between the commits 893613a and 204ffb6 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

sourcegraph-bot · 2022-10-17T21:16:23Z

Codenotify: Notifying subscribers in OWNERS files for diff 204ffb6...893613a.

Notify	File(s)
@sourcegraph/frontend-platform-devs	client/wildcard/src/components/Charts/core/components/tooltip/Tooltip.tsx client/wildcard/src/components/Combobox/Combobox.module.scss client/wildcard/src/components/Combobox/Combobox.tsx client/wildcard/src/components/NavMenu/NavMenu.test.tsx client/wildcard/src/components/Popover/Popover.tsx client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx client/wildcard/src/components/Popover/tether/services/tether-registry.ts client/wildcard/src/components/index.ts

- 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 group option layout - Fix scroll into view logic (add scroll into center functionality)

… listbox)

chwarwick

The insights related changes look fine to me. I don't like that you need to append numbers to duplicate dashboard names and that they would be inconsistent between users.

chwarwick · 2022-10-19T17:42:38Z

...eb/src/enterprise/insights/core/backend/gql-backend/methods/get-dashboards/get-dashboards.ts

@@ -46,7 +47,22 @@ export const getDashboards = (apolloClient: ApolloClient<unknown>, id?: string):
        })
    )

-export function deserializeDashboardsOwners(
+function makeDashboardTitleUnuque(dashboards: InsightsDashboardNode[]): InsightsDashboardNode[] {


I'm not sure how I feel about this. I assume this would not be common, but also it's user specific so my Chris Dashboard might be your Chris Dashboard 2.

Yeah, you're right that there is a possibility to have non-consistent dashboard names. I don't like this behaviour either. I did this because reach ui combobox doesn't work with non unique items. I think this problem is bad anyway (with number suffix or without)

I believe if someone wants to share a dashboard they would probably go with dashboard URL where we have unique id instead of copy past title

Even if we someone uses title as something for looking for a dashboard I think even without numbers it would confuse users, without number they will see different amount of dashboards, so inconsistent problem persists here.

Agree two with the same name is just as bad so I don't consider this a blocker if the change brings other positive benefits.

chwarwick · 2022-10-19T17:59:17Z

I haven't been able to get the web preview link to work for this, I get an error so I could try it to get a sense of how it feels to use aside from playing in the storybook, which worked well for me.

vovakulikov · 2022-10-19T18:33:49Z

@chwarwick the web app link is broken because of redirect issue
The right link is https://sg-web-vk-improve-dashboard-select.onrender.com/search

chwarwick · 2022-10-19T18:57:10Z

@chwarwick the web app link is broken because of redirect issue The right link is https://sg-web-vk-improve-dashboard-select.onrender.com/search

Thanks @vovakulikov that worked and I didn't have any issues with dashboard select. Not sure if it's related to this change but I keep getting stuck in the screen reader after changing dashboards it repeats "Loading, Loading, Loading.... ".

vovakulikov · 2022-10-19T18:59:51Z

@chwarwick thanks for checking implementation! Yeah, this issue with loading state that's the infamous, here is an issue for this https://github.com/sourcegraph/sourcegraph/issues/41684

AlicjaSuska

Thank you for making this improvement @vovakulikov! It works really nicely, the keyboard navigation is smooth. No issues from my side. Well done!

I've noticed that the three dots menu icon for dashboard context menu has the incorrect color:

it should be $icon-color
it is: $body-color in light mode
renders as a dark grey in dark mode, even though it has the $body-color color assigned

I'm not sure if it's connected to any changes made in this PR, but could you please help to change this? Thank you!

chwarwick

From the insights side 👍

vovakulikov · 2022-10-20T22:15:17Z

Thanks for UI and tech review! The issue with three dot context menu and the dark theme is covered in a separate issue, I will address it in a next PR.

philipp-spiess · 2022-10-24T12:17:11Z

It seems like this change has introduced an endless recursion in combination with the git blame view (maybe other views as well?): https://github.com/sourcegraph/sourcegraph/issues/43335#issuecomment-1288942023

vovakulikov self-assigned this Oct 17, 2022

cla-bot bot added the cla-signed label Oct 17, 2022

vovakulikov requested a review from AlicjaSuska October 17, 2022 21:13

vovakulikov marked this pull request as ready for review October 17, 2022 21:14

vovakulikov added 17 commits October 18, 2022 11:40

Refactor Popover UI

838972d

- 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

cc55989

- Fix group option layout - Fix scroll into view logic (add scroll into center functionality)

Connect forward ref and truncated text element

4883181

Make all dashboard names unique

54622c5

Refactor DashboardSelect UI (use combobox + popover over the reach ui…

f3b2d28

… listbox)

Remove legacy dashboard select UI components

4d6db1c

Remove @reach/listbox styles from the main style entry

cb0dd86

Fix dashboard content import

903203e

Fix combobox popover close logic

24798ff

Fix dashboard page unit tests

65e0a6b

Update dashboard select story

734e007

Fix unit tests of UI components that use popover UI

28c1414

Update feedback prompt unit tests

dc5275d

Fix chart tooltip return to target logic

32971be

Fix insight integration tests (dashboard select selectors)

6b7f0f6

Fix layout and dashboard option selectors

41e8b9e

Bring back async tether scheduling

d068b76

vovakulikov force-pushed the vk/improve-dashboard-select branch from 45d0a8e to d068b76 Compare October 18, 2022 14:46

vovakulikov requested review from valerybugakov and chwarwick October 18, 2022 14:51

vovakulikov added 2 commits October 18, 2022 14:31

Fix jest snapshots

57c6087

Remove reach ui listbox package

893613a

chwarwick reviewed Oct 19, 2022

View reviewed changes

AlicjaSuska approved these changes Oct 20, 2022

View reviewed changes

chwarwick approved these changes Oct 20, 2022

View reviewed changes

vovakulikov merged commit 05b188c into main Oct 20, 2022

vovakulikov deleted the vk/improve-dashboard-select branch October 20, 2022 22:15

philipp-spiess mentioned this pull request Oct 24, 2022

UI error #43335

Closed

vovakulikov mentioned this pull request Oct 24, 2022

Blob UI: Disable return to target logic in blame popover UI #43353

Merged

vovakulikov mentioned this pull request Nov 8, 2022

[Accessibility]: Branch/Tag/Commit + Repositories selector: Focus lost on ESC key #35476

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Insights: Improve dashboard select UI #43029

Code Insights: Improve dashboard select UI #43029

vovakulikov commented Oct 17, 2022 •

edited by AlicjaSuska

Loading

sg-e2e-regression-test-bob commented Oct 17, 2022 •

edited

Loading

sourcegraph-bot commented Oct 17, 2022 •

edited

Loading

chwarwick left a comment

chwarwick Oct 19, 2022

vovakulikov Oct 19, 2022

chwarwick Oct 19, 2022

chwarwick commented Oct 19, 2022

vovakulikov commented Oct 19, 2022

chwarwick commented Oct 19, 2022

vovakulikov commented Oct 19, 2022

AlicjaSuska left a comment

chwarwick left a comment

vovakulikov commented Oct 20, 2022

philipp-spiess commented Oct 24, 2022

Code Insights: Improve dashboard select UI #43029

Code Insights: Improve dashboard select UI #43029

Conversation

vovakulikov commented Oct 17, 2022 • edited by AlicjaSuska Loading

Problem

Solution

Tradeoffs

Test plan

App preview:

sg-e2e-regression-test-bob commented Oct 17, 2022 • edited Loading

Bundle size report 📦

sourcegraph-bot commented Oct 17, 2022 • edited Loading

chwarwick left a comment

Choose a reason for hiding this comment

chwarwick Oct 19, 2022

Choose a reason for hiding this comment

vovakulikov Oct 19, 2022

Choose a reason for hiding this comment

chwarwick Oct 19, 2022

Choose a reason for hiding this comment

chwarwick commented Oct 19, 2022

vovakulikov commented Oct 19, 2022

chwarwick commented Oct 19, 2022

vovakulikov commented Oct 19, 2022

AlicjaSuska left a comment

Choose a reason for hiding this comment

chwarwick left a comment

Choose a reason for hiding this comment

vovakulikov commented Oct 20, 2022

philipp-spiess commented Oct 24, 2022

vovakulikov commented Oct 17, 2022 •

edited by AlicjaSuska

Loading

sg-e2e-regression-test-bob commented Oct 17, 2022 •

edited

Loading

sourcegraph-bot commented Oct 17, 2022 •

edited

Loading