Skip to content

Commit

Permalink
Merge pull request #1986 from iterative/checkbox-selection-issues
Browse files Browse the repository at this point in the history 
Fix checkbox selection issues
  • Loading branch information
@wolmir
wolmir authored Jul 7, 2022
2 parents 5edaf3d + 40c4f56 commit b30eedf
Show file tree
Hide file tree
Showing 6 changed files with 279 additions and 48 deletions.
133 changes: 132 additions & 1 deletion webview/src/experiments/components/App.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -746,8 +746,8 @@ describe('App', () => {
const menuitems = screen.getAllByRole('menuitem')
const itemLabels = menuitems.map(item => item.textContent)
expect(itemLabels).toStrictEqual([
'Modify and Resume',
'Modify, Reset and Run',
'Modify and Resume',
'Modify and Queue',
'Star Experiment'
])
Expand Down Expand Up @@ -842,6 +842,137 @@ describe('App', () => {
const itemLabels = menuitems.map(item => item.textContent)
expect(itemLabels).toContain('Star Experiments')
})

it('should allow batch selection from the bottom up too', () => {
render(<App />)

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

const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox')
const tailRow = within(getRow('42b8736')).getByRole('checkbox')
fireEvent.click(tailRow)
fireEvent.click(firstRowCheckbox, { shiftKey: true })

const selectedRows = () => screen.getAllByRole('row', { selected: true })
expect(selectedRows()).toHaveLength(4)

const anotherRow = within(getRow('2173124')).getByRole('checkbox')
fireEvent.click(anotherRow, { shiftKey: true })
expect(selectedRows()).toHaveLength(5)
})

it('should not include collapsed experiments in the bulk selection', () => {
render(<App />)

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

const testBranchContractButton = within(getRow('42b8736')).getByTitle(
'Contract Row'
)
fireEvent.click(testBranchContractButton)

jest.advanceTimersByTime(100)
const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox')
fireEvent.click(firstRowCheckbox)

const tailRow = within(getRow('22e40e1')).getByRole('checkbox')
fireEvent.click(tailRow, { shiftKey: true })

fireEvent.click(testBranchContractButton)

jest.advanceTimersByTime(100)
expect(getRow('42b8736')).toHaveAttribute('aria-selected', 'true')
expect(getRow('2173124')).not.toHaveAttribute('aria-selected', 'true')
expect(getRow('9523bde')).not.toHaveAttribute('aria-selected', 'true')
})

it('should present the Clear selected rows option when multiple rows are selected', () => {
render(<App />)

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

const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox')
fireEvent.click(firstRowCheckbox)

const tailRow = within(getRow('42b8736')).getByRole('checkbox')
fireEvent.click(tailRow, { shiftKey: true })

const selectedRows = () =>
screen.queryAllByRole('row', { selected: true })
expect(selectedRows().length).toBe(4)

const target = screen.getByText('4fb124a')
fireEvent.contextMenu(target, { bubbles: true })

jest.advanceTimersByTime(100)
const clearOption = screen.getByText('Clear row selection')
fireEvent.click(clearOption)

jest.advanceTimersByTime(100)
expect(selectedRows().length).toBe(0)
})

it('should clear the row selection when the Escape key is pressed', () => {
render(<App />)

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

const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox')
fireEvent.click(firstRowCheckbox)

const tailRow = within(getRow('42b8736')).getByRole('checkbox')
fireEvent.click(tailRow, { shiftKey: true })

const selectedRows = () =>
screen.queryAllByRole('row', { selected: true })
expect(selectedRows().length).toBe(4)

fireEvent.keyUp(tailRow, { bubbles: true, key: 'Escape' })

jest.advanceTimersByTime(100)
expect(selectedRows().length).toBe(0)
})
})

describe('Star Experiments', () => {
Expand Down
99 changes: 74 additions & 25 deletions webview/src/experiments/components/table/Row.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ const experimentMenuOption = (
} as MessagesMenuOptionProps
}

const getMultiSelectMenuOptions = (selectedRowsList: RowProp[]) => {
const getMultiSelectMenuOptions = (
selectedRowsList: RowProp[],
hasRunningExperiment: boolean
) => {
const unstarredExperiments = selectedRowsList.filter(
({
row: {
Expand All @@ -67,7 +70,8 @@ const getMultiSelectMenuOptions = (selectedRowsList: RowProp[]) => {
.filter(value => value.row.depth === 1)
.map(value => value.row.values.id)

const hideRemoveOption = removableRowIds.length !== selectedRowsList.length
const hideRemoveOption =
removableRowIds.length !== selectedRowsList.length || hasRunningExperiment

const toggleStarOption = (ids: string[], label: string) =>
experimentMenuOption(
Expand All @@ -92,6 +96,46 @@ const getMultiSelectMenuOptions = (selectedRowsList: RowProp[]) => {
MessageFromWebviewType.REMOVE_EXPERIMENT,
hideRemoveOption,
true
),
{
divider: true,
id: 'clear-selection',
keyboardShortcut: 'Esc',
label: 'Clear row selection'
}
]
}

const getRunResumeOptions = (
withId: (
label: string,
type: MessageFromWebviewType,
hidden?: boolean,
divider?: boolean
) => MessagesMenuOptionProps,
isWorkspace: boolean,
projectHasCheckpoints: boolean,
hideVaryAndRun: boolean,
depth: number
) => {
const isNotCheckpoint = depth <= 1 || isWorkspace

return [
withId(
'Modify, Reset and Run',
MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_RESET_AND_RUN,
!isNotCheckpoint || !projectHasCheckpoints
),
withId(
projectHasCheckpoints ? 'Modify and Resume' : 'Modify and Run',
MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN,
!isNotCheckpoint,
!hideVaryAndRun
),
withId(
'Modify and Queue',
MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_QUEUE,
!isNotCheckpoint
)
]
}
Expand All @@ -100,53 +144,51 @@ const getSingleSelectMenuOptions = (
id: string,
isWorkspace: boolean,
projectHasCheckpoints: boolean,
hasRunningExperiment: boolean,
depth: number,
queued?: boolean,
starred?: boolean
) => {
const isNotCheckpoint = depth <= 1 || isWorkspace
const canApplyOrCreateBranch = queued || isWorkspace || depth <= 0
const hideApplyAndCreateBranch = queued || isWorkspace || depth <= 0

const withId = (
label: string,
type: MessageFromWebviewType,
hidden?: boolean,
divider?: boolean
) => experimentMenuOption(id, label, type, hidden, divider)
) =>
experimentMenuOption(
id,
label,
type,
hidden || hasRunningExperiment,
divider
)

return [
withId(
'Apply to Workspace',
MessageFromWebviewType.APPLY_EXPERIMENT_TO_WORKSPACE,
canApplyOrCreateBranch
hideApplyAndCreateBranch
),
withId(
'Create new Branch',
MessageFromWebviewType.CREATE_BRANCH_FROM_EXPERIMENT,
canApplyOrCreateBranch
),
withId(
projectHasCheckpoints ? 'Modify and Resume' : 'Modify and Run',
MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN,
!isNotCheckpoint,
!canApplyOrCreateBranch
hideApplyAndCreateBranch
),
withId(
'Modify, Reset and Run',
MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_RESET_AND_RUN,
!isNotCheckpoint || !projectHasCheckpoints
),
withId(
'Modify and Queue',
MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_QUEUE,
!isNotCheckpoint
...getRunResumeOptions(
withId,
isWorkspace,
projectHasCheckpoints,
hideApplyAndCreateBranch,
depth
),
experimentMenuOption(
[id],
starred ? 'Unstar Experiment' : 'Star Experiment',
MessageFromWebviewType.TOGGLE_EXPERIMENT_STAR,
isWorkspace,
true
!hasRunningExperiment
),
withId(
'Remove',
Expand All @@ -161,6 +203,7 @@ const getContextMenuOptions = (
id: string,
isWorkspace: boolean,
projectHasCheckpoints: boolean,
hasRunningExperiment: boolean,
depth: number,
selectedRows: Record<string, RowProp | undefined>,
queued?: boolean,
Expand All @@ -173,12 +216,13 @@ const getContextMenuOptions = (

return cond(
isFromSelection && selectedRowsList.length > 1,
() => getMultiSelectMenuOptions(selectedRowsList),
() => getMultiSelectMenuOptions(selectedRowsList, hasRunningExperiment),
() =>
getSingleSelectMenuOptions(
id,
isWorkspace,
projectHasCheckpoints,
hasRunningExperiment,
depth,
queued,
starred
Expand All @@ -187,6 +231,7 @@ const getContextMenuOptions = (
}

export const RowContextMenu: React.FC<RowProp> = ({
hasRunningExperiment = false,
projectHasCheckpoints = false,
row: {
original: { queued, starred },
Expand All @@ -204,6 +249,7 @@ export const RowContextMenu: React.FC<RowProp> = ({
id,
isWorkspace,
projectHasCheckpoints,
hasRunningExperiment,
depth,
selectedRows,
queued,
Expand All @@ -216,7 +262,8 @@ export const RowContextMenu: React.FC<RowProp> = ({
depth,
id,
projectHasCheckpoints,
selectedRows
selectedRows,
hasRunningExperiment
])

return (
Expand Down Expand Up @@ -267,6 +314,7 @@ export const RowContent: React.FC<
changes,
contextMenuDisabled,
projectHasCheckpoints,
hasRunningExperiment,
batchRowSelection
}): JSX.Element => {
const {
Expand Down Expand Up @@ -319,6 +367,7 @@ export const RowContent: React.FC<
<RowContextMenu
row={row}
projectHasCheckpoints={projectHasCheckpoints}
hasRunningExperiment={hasRunningExperiment}
/>
}
>
Expand Down
Loading

0 comments on commit b30eedf

Please sign in to comment.