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

Jump to bottom

Fix checkbox selection issues #1986

Merged
merged 8 commits into from
Jul 7, 2022
Merged

Fix checkbox selection issues #1986

merged 8 commits into from
Jul 7, 2022

Conversation

wolmir
Copy link
Contributor

@wolmir wolmir commented Jul 5, 2022 •
edited
Loading

This PR will fix some issues with the current checkbox selection of rows in the experiments table.

It will also be possible to clear the selection with the Esc key.

Screen.Recording.2022-07-05.at.16.21.31.mov

TODO

  • Remove the context-menu suppression when an experiment is running (apply rule per-option)
  • Only select the parent row if all of its children are selected

@wolmir wolmir requested review from maxagin and shcheklein July 5, 2022 18:38
@wolmir wolmir mentioned this pull request Jul 5, 2022
Closed
2 tasks
@maxagin
Copy link
Contributor

maxagin commented Jul 5, 2022

Do we have somewhere documented list of the supported hotkeys? How the users will know about this option?
We may want to think about educating the user in regards to this small, but important details and functionality.

@wolmir
Copy link
Contributor Author

wolmir commented Jul 5, 2022

Do we have somewhere documented list of the supported hotkeys? How the users will know about this option?
We may want to think about educating the user in regards to this small, but important details and functionality.

I'll add the option to the context-menu as well

What about showing the keyboard shortcut (Esc) in that option @maxagin ?

@maxagin
Copy link
Contributor

maxagin commented Jul 5, 2022 •
edited
Loading

Unselect selected + the keyboard shortcut (Esc) for the context menu. Good idea @wolmir I think!
Great 👍

@wolmir
Copy link
Contributor Author

wolmir commented Jul 5, 2022

@maxagin I updated the video above with the menu option

@wolmir wolmir force-pushed the checkbox-selection-issues branch from 47e0607 to 18e908d Compare July 5, 2022 19:47
@mattseddon mattseddon added the bug Something isn't working label Jul 6, 2022
@mattseddon
Copy link
Member

Do we have somewhere documented list of the supported hotkeys? How the users will know about this option? We may want to think about educating the user in regards to this small, but important details and functionality.

@maxagin https://code.visualstudio.com/docs/getstarted/keybindings

mattseddon
mattseddon reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

Have gone through and left some comments, IMO we will want to suppress the selection of checkpoints on collapsed experiments before merging:

Screen.Recording.2022-07-06.at.11.46.28.am.mov

webview/src/experiments/components/App.test.tsx

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.

webview/src/experiments/components/table/Row.tsx
@@ -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.

webview/src/experiments/components/table/Row.tsx Outdated
!hideVaryAndRun
),
withId(
'Modify, Reset and Run',
Copy link
Member

Choose a reason for hiding this comment

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

[Q] Can we reorder these to match the tree context menu?

image

Copy link
Member

Choose a reason for hiding this comment

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

The reason being "Reset and Run" is the most likely candidate for the next action.

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, that seems like a simple change

webview/src/experiments/components/table/Row.tsx

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

webview/src/experiments/components/table/Row.tsx
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.

webview/src/experiments/components/table/Row.tsx Outdated
]
}

const getWorkspaceOptions = (
Copy link
Member

Choose a reason for hiding this comment

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

[C] Doesn't appear that this only applies to the workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll correct the name

webview/src/experiments/components/table/Table.tsx
@@ -139,29 +147,37 @@ export const Table: React.FC<TableProps & WithChanges> = ({
useClickOutside(tableRef, clickOutsideHandler)

const batchRowSelection = React.useCallback(
Copy link
Member

Choose a reason for hiding this comment

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

[Q] Can we further test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I think I can add some more tests today

@maxagin
Copy link
Contributor

maxagin commented Jul 6, 2022

Do we have somewhere documented list of the supported hotkeys? How the users will know about this option? We may want to think about educating the user in regards to this small, but important details and functionality.

@maxagin https://code.visualstudio.com/docs/getstarted/keybindings

Thanks @mattseddon ! But my question was more related to how and when we decided to use the ESC.

@mattseddon
Copy link
Member

If you followed the instructions in the link you would be able to see all of the actions in VS Code currently mapped to Escape

image

Hint: generally they relate to closing/unselecting/hiding

@wolmir wolmir force-pushed the checkbox-selection-issues branch from 25eedbf to 4aa3583 Compare July 6, 2022 17:23
@codeclimate
Copy link

codeclimate bot commented Jul 6, 2022

Code Climate has analyzed commit 40c4f56 and detected 7 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 7

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.6% (0.0% change).

View more on Code Climate.

@rogermparent
Copy link
Contributor

rogermparent commented Jul 7, 2022 •
edited
Loading

context-menu-esc-demo.mp4

I think it'd be best to make the tooltips dismiss on esc (maybe even for all our interactive tooltips). I don't think it's a blocker but warrants a follow-up or an issue.

rogermparent
rogermparent approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

I think this comment is both important and simple enough to be considered before merging, but I don't see any other showstoppers right now. Nice work!

@wolmir wolmir merged commit b30eedf into main Jul 7, 2022
@wolmir wolmir deleted the checkbox-selection-issues branch July 7, 2022 18:44
@shcheklein
Copy link
Member

@wolmir good stuff! 😎 please create a housekeeping / followup ticket for the comments / improvements we need to address still.

@mattseddon mattseddon mentioned this pull request Jul 14, 2022
Closed
@wolmir wolmir mentioned this pull request Jul 14, 2022
Closed
3 tasks
@mattseddon mattseddon mentioned this pull request Sep 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattseddon mattseddon mattseddon left review comments

@rogermparent rogermparent rogermparent approved these changes

@sroy3 sroy3 Awaiting requested review from sroy3

@maxagin maxagin Awaiting requested review from maxagin

@shcheklein shcheklein Awaiting requested review from shcheklein shcheklein is a code owner

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wolmir @maxagin @mattseddon @rogermparent @shcheklein