Skip to content
Merged
Show file tree
Hide file tree
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
124 changes: 124 additions & 0 deletions webview/src/experiments/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
import { getRow } from '../../test/queries'
import { dragAndDrop } from '../../test/dragDrop'
import { DragEnterDirection } from '../../shared/components/dragDrop/util'
import { setExperimentsAsStarred } from '../../test/tableDataFixture'

jest.mock('../../shared/api')
jest.mock('../../util/styles')
Expand Down Expand Up @@ -372,6 +373,129 @@ describe('App', () => {
})
})

describe('Sub-rows middle states indicators', () => {
const renderWithAllRowsExpanded = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

[I] A more central location for these util will be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting out utils will help to start reducing the size of this file and would give a central location for people to look for test building blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

fine to create a separate PR for that

render(<App />)

fireEvent(
window,
new MessageEvent('message', {
data: {
data: tableDataFixture,
type: MessageToWebviewType.SET_DATA
}
})
)
}

const getMiddleStatesTestRow = () => {
return getRow('4fb124a')
}

const getCountIndicators = (element: HTMLElement) => {
return within(element).queryAllByRole('marquee')
}

const getCountIndicatorById = (row: HTMLElement, rowActionId: string) => {
const checkboxRowAction = within(row).getByTestId(rowActionId)

return within(checkboxRowAction).queryByRole('marquee')
}

const getCheckboxCountIndicator = (row: HTMLElement) => {
return getCountIndicatorById(row, 'row-action-checkbox')
}

const clickRowCheckbox = (label: string) => {
const checkbox = within(getRow(label)).getByRole('checkbox')

fireEvent.click(checkbox)
}

const selectSomeSubRows = () => {
clickRowCheckbox('d1343a8')
clickRowCheckbox('1ee5f2e')

return 2
}

const starSomeSubRows = () => {
const starredFixture = setExperimentsAsStarred(tableDataFixture, [
'd1343a8',
'1ee5f2e'
])

fireEvent(
window,
new MessageEvent('message', {
data: {
data: starredFixture,
type: MessageToWebviewType.SET_DATA
}
})
)

return 2
}

const toggleExpansion = (row: HTMLElement) => {
const expandButton = within(row).getByTitle('Contract Row')

fireEvent.click(expandButton)
}

it('should be hidden when the parent row is expanded', () => {
renderWithAllRowsExpanded()
const row = getMiddleStatesTestRow()
const indicators = getCountIndicators(row)
expect(indicators).toHaveLength(0)
})

describe('Checkbox selection counter', () => {
it('should not be visible if no sub-row was checked', () => {
renderWithAllRowsExpanded()
const row = getMiddleStatesTestRow()
const indicator = getCheckboxCountIndicator(row)
expect(indicator).not.toBeInTheDocument()
})

it('should display the correct number of checked sub-rows when the parent is collapsed', () => {
renderWithAllRowsExpanded()
const row = getMiddleStatesTestRow()
const numberOfSubrowsSelected = selectSomeSubRows()
expect(getCheckboxCountIndicator(row)).not.toBeInTheDocument()
toggleExpansion(row)
const collapsed = getMiddleStatesTestRow()
expect(getCheckboxCountIndicator(collapsed)).toHaveTextContent(
`${numberOfSubrowsSelected}`
)
})
})

describe('Stars counter', () => {
it('should not be visible if no sub-row was starred', () => {
renderWithAllRowsExpanded()
const row = getMiddleStatesTestRow()
const indicator = getCountIndicatorById(row, 'row-action-star')
expect(indicator).not.toBeInTheDocument()
})

it('should display the correct number of starred sub-rows when the parent is collapsed', () => {
renderWithAllRowsExpanded()
const row = getMiddleStatesTestRow()
const numberOfSubrowsStarred = starSomeSubRows()
expect(
getCountIndicatorById(row, 'row-action-star')
).not.toBeInTheDocument()
toggleExpansion(row)
const collapsed = getMiddleStatesTestRow()
expect(
getCountIndicatorById(collapsed, 'row-action-star')
).toHaveTextContent(`${numberOfSubrowsStarred}`)
})
})
})

describe('Toggle experiment status', () => {
it('should send a message to the extension to toggle an experiment when the row is clicked', () => {
render(<App />)
Expand Down
3 changes: 2 additions & 1 deletion webview/src/experiments/components/Experiments.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ const getColumns = (columns: Column[]): TableColumn<Row>[] =>
Header: ExperimentHeader,
accessor: 'id',
id: 'id',
width: 150
minWidth: 250,
width: 250
},
{
Cell: ({ value }: { value: string }) => {
Expand Down
140 changes: 111 additions & 29 deletions webview/src/experiments/components/table/Cell.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import React from 'react'
import cx from 'classnames'
import { VSCodeCheckbox } from '@vscode/webview-ui-toolkit/react'
import { Indicator, IndicatorWithJustTheCounter } from './Indicators'
import styles from './styles.module.scss'
import { CellProp, RowProp } from './interfaces'
import ClockIcon from '../../../shared/components/icons/Clock'
import { clickAndEnterProps } from '../../../util/props'
import { StarFull, StarEmpty } from '../../../shared/components/icons'
import { pluralize } from '../../../util/strings'

const RowExpansionButton: React.FC<RowProp> = ({ row }) =>
row.canExpand ? (
Expand All @@ -31,27 +33,115 @@ const RowExpansionButton: React.FC<RowProp> = ({ row }) =>
<span className={styles.rowArrowContainer} />
)

export const FirstCell: React.FC<
CellProp & {
bulletColor?: string
isRowSelected: boolean
toggleExperiment: () => void
toggleRowSelection: () => void
toggleStarred: () => void
export type RowActionsProps = {
isRowSelected: boolean
showSubRowStates: boolean
starred?: boolean
subRowStates: {
selections: number
stars: number
plotSelections: number
}
> = ({
cell,
bulletColor,
toggleRowSelection: () => void
toggleStarred: () => void
}

export type RowActionProps = {
showSubRowStates: boolean
subRowsAffected: number
actionAppliedLabel: string
children: React.ReactElement
hidden?: boolean
testId?: string
}

export const RowAction: React.FC<RowActionProps> = ({
showSubRowStates,
subRowsAffected,
actionAppliedLabel,
children,
hidden,
testId
}) => {
const count = (showSubRowStates && subRowsAffected) || 0

return (
<div
className={cx(styles.rowActions, hidden && styles.hidden)}
data-testid={testId}
>
<Indicator
count={count}
tooltipContent={
count &&
`${count} ${pluralize('sub-row', count)} ${actionAppliedLabel}.`
}
>
{children}
Copy link
Contributor

Choose a reason for hiding this comment

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

[I] At the moment only the direct children are being aggregated. Here is a screen recording of an example:

Screen.Recording.2022-07-12.at.8.30.37.pm.mov

When we aggregate from checkpoints to experiments and then up to the current commit we are not taking the cumulative total into account.

Another example:

Screen.Recording.2022-07-12.at.8.35.31.pm.mov

I would not even try to include the top level indicator until we have more than one commit/branch in the table. Until that time we will be catering to an edge case.

However, if you feel that they are needed then they need to reflect the totals including all children.

Copy link
Contributor

Choose a reason for hiding this comment

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

More complicated aggregation behaviour will also need some tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add some tests for this current use-case now, but I want to defer this total aggregation for later if it's currently an edge case.

</Indicator>
</div>
)
}

export const RowActions: React.FC<RowActionsProps> = ({
isRowSelected,
toggleExperiment,
showSubRowStates,
starred,
subRowStates: { selections, stars },
toggleRowSelection,
toggleStarred
}) => {
return (
<>
<RowAction
showSubRowStates={showSubRowStates}
subRowsAffected={selections}
actionAppliedLabel={'selected'}
testId={'row-action-checkbox'}
>
<VSCodeCheckbox
{...clickAndEnterProps(toggleRowSelection)}
checked={isRowSelected}
/>
</RowAction>
<RowAction
showSubRowStates={showSubRowStates}
subRowsAffected={stars}
actionAppliedLabel={'starred'}
testId={'row-action-star'}
>
<div
className={styles.starSwitch}
role="switch"
aria-checked={starred}
tabIndex={0}
{...clickAndEnterProps(toggleStarred)}
data-testid="star-icon"
>
{starred && <StarFull />}
{!starred && <StarEmpty />}
</div>
</RowAction>
</>
)
}

export const FirstCell: React.FC<
CellProp &
RowActionsProps & {
bulletColor?: string
toggleExperiment: () => void
}
> = ({ cell, bulletColor, toggleExperiment, ...rowActionsProps }) => {
const { row, isPlaceholder } = cell
const {
original: { starred, queued }
original: { queued }
} = row

const {
subRowStates: { plotSelections }
} = rowActionsProps

return (
<div
{...cell.getCellProps({
Expand All @@ -63,29 +153,21 @@ export const FirstCell: React.FC<
})}
>
<div className={styles.innerCell}>
<div className={styles.rowActions}>
<VSCodeCheckbox
{...clickAndEnterProps(toggleRowSelection)}
checked={isRowSelected}
/>
<div
className={styles.starSwitch}
role="switch"
aria-checked={starred}
tabIndex={0}
{...clickAndEnterProps(toggleStarred)}
data-testid="star-icon"
>
{starred && <StarFull />}
{!starred && <StarEmpty />}
</div>
</div>
<RowActions {...rowActionsProps} />
<RowExpansionButton row={row} />
<span
className={styles.bullet}
style={{ color: bulletColor }}
{...clickAndEnterProps(toggleExperiment)}
>
<IndicatorWithJustTheCounter
aria-label={'Sub-rows selected for plots'}
count={plotSelections}
tooltipContent={`${plotSelections} ${pluralize(
'sub-row',
plotSelections
)} selected for plots.`}
/>
{queued && <ClockIcon />}
</span>
{isPlaceholder ? null : (
Expand Down
Loading