Implement checkbox selection #1964
Conversation
wolmir
requested review from
rogermparent,
sroy3,
maxagin and
shcheklein
and removed request for
sroy3 and
rogermparent
|
@rogermparent @mattseddon Can you take a look at this, please? |
|
Code Climate has analyzed commit d94de1d and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.5% (0.0% change). View more on Code Climate. |
| const toggleRowSelection = React.useCallback<HandlerFunc<HTMLElement>>( | ||
| args => { | ||
| if (!isWorkspace) { | ||
| if (args?.mouse?.shiftKey) { |
There was a problem hiding this comment.
[Q] How do we implement the reverse? After doing a bulk selection I am unable to bulk unselect. Instead, I have to unclick every row manually.
Screen.Recording.2022-07-04.at.8.50.28.am.mov
There was a problem hiding this comment.
I may be doing it wrong but I could not work it out.
There was a problem hiding this comment.
Also, get some weird bulk select behavior when the row is collapsed:
Screen.Recording.2022-07-04.at.8.55.46.am.mov
For the first collapsed experiment the first checkpoint is selected and for the last experiment, no checkpoints are selected. I would expect none or all of the checkpoints to be selected when I perform this action. For our particular checkbox use case, I would expect none of the checkpoints to be selected.
There was a problem hiding this comment.
[Q] How do we implement the reverse? After doing a bulk selection I am unable to bulk unselect. Instead, I have to unclick every row manually.
Good point. I have commented this here: #1924 (comment)
In the case of multiple selections, should not we have Unselect All option in the menu?
There was a problem hiding this comment.
I like the Esc option, it's the first key I tried to hit as well.
I discussed an idea with Matt yesterday, but I didn't like it in the end.
| @@ -43,6 +50,7 @@ export const ExperimentGroup: React.FC<RowProp & InstanceProp> = ({ | |||
| instance={instance} | |||
| contextMenuDisabled={contextMenuDisabled} | |||
There was a problem hiding this comment.
Seems like we are still suppressing the full context menu whenever an experiment is running. Perhaps we need to now do this on an action-by-action basis as there is nothing to stop a user from "starring" multiple experiments on the fly whilst an experiment is running.
| jest.advanceTimersByTime(100) | ||
| const menuitems = screen.getAllByRole('menuitem') | ||
| const itemLabels = menuitems.map(item => item.textContent) | ||
| expect(itemLabels).toContain('Star Experiments') |
There was a problem hiding this comment.
[C] Could click the button and check the correct message is fired here.
|
I would also have at least one row selected and one row starred in the storybook for snapshot testing. You should be able to use the play functionality to achieve that. |
#1924 and #1960
This PR will add checkboxes for row selection to the Experiments table webview.
It will also allow batch selection of rows by shift-clicking a range of them (see demo)
Screen.Recording.2022-06-30.at.17.48.50.mov
TODO