Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix checkbox selection issues #1986

Merged
merged 8 commits into from
Jul 7, 2022
Merged

Fix checkbox selection issues #1986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9e529ba
Clear row selection on Esc
wolmir Jul 5, 2022
3afa2ae
Fix bulk-selection
wolmir Jul 5, 2022
7dfd915
Add ctx-menu option to clear row selection
wolmir Jul 5, 2022
c335c91
Enable specific options when an experiment is running
wolmir Jul 5, 2022
4aa3583
Prevent the bulk selection of collapsed checkpoints
wolmir Jul 6, 2022
352e7fb
Fix function name
wolmir Jul 6, 2022
0605411
Change the options order to match the tree options
wolmir Jul 6, 2022
40c4f56
Add some more tests for bulk selection
wolmir Jul 6, 2022
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
133 changes: 132 additions & 1 deletion webview/src/experiments/components/App.test.tsx
View file
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

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

[N] I think you can use toHaveLength here.


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
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

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

[C] IMO if one of the selected rows is removable we should offer the remove option and remove only the ones we can.


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,
Copy link
Member

Choose a reason for hiding this comment

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

[C] Might be easier to read if you change the condition to isCheckpoint instead of the double negative.

Copy link
Member

Choose a reason for hiding this comment

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

try and simplify this boolean logic wherever you can, it is getting pretty difficult to understand at first glance.

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 don't like it either, but I have a feeling this refactoring will take some time. I can tackle it in the next PR, if that's ok.

!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
Copy link
Member

Choose a reason for hiding this comment

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

[Q] The name would suggest the logic has been reversed here (but it looks like the same condition). Was the name wrong before and the logic correct or has something else happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because the property is called hidden, so these are assertions that will hide the option when true. It was wrong before

),
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