Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Code Insights: Improve dashboard select UI #43029

Merged
merged 19 commits into from
Oct 20, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion client/web/src/SourcegraphWebApp.scss
Original file line number Diff line number Diff line change
@@ -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';
Original file line number Diff line number Diff line change
@@ -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'>
Original file line number Diff line number Diff line change
@@ -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[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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[] {
Original file line number Diff line number Diff line change
@@ -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')
})
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
@@ -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'
Loading