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.

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

182849961 table advance selection #1527

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

eireland
Copy link
Contributor

@eireland eireland commented Oct 1, 2024

Adds ability to select multiple cases in the table via:

  1. Keyboard navigation - use modifier keys (shift, command, alt) + arrow up or down keys to add additional cases to be selected after user clicks on a case.
  2. Marquee select - click and drag over table grid to select cases. Cases are deselected if user changes direction from initial direction of drag.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 58.33333% with 40 lines in your changes missing coverage. Please review.

Project coverage is 85.13%. Comparing base (e95d860) to head (b410029).
Report is 45 commits behind head on main.

Files with missing lines Patch % Lines
v3/src/components/case-table/collection-table.tsx 56.04% 40 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1527      +/-   ##
==========================================
- Coverage   85.29%   85.13%   -0.17%     
==========================================
  Files         560      560              
  Lines       27952    28041      +89     
  Branches     7180     7722     +542     
==========================================
+ Hits        23842    23872      +30     
- Misses       3954     4013      +59     
  Partials      156      156              
Flag Coverage Δ
cypress 74.40% <59.55%> (-0.15%) ⬇️
jest 53.39% <16.66%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

cypress bot commented Oct 1, 2024

codap-v3    Run #4502

Run Properties:  status check passed Passed #4502  •  git commit b410029327: changes the condition to check against for deleting cases
Project codap-v3
Branch Review 182849961-table-advance-selection
Run status status check passed Passed #4502
Run duration 08m 34s
Commit git commit b410029327: changes the condition to check against for deleting cases
Committer eireland
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 30
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 221
View all changes introduced in this branch ↗︎

@kswenson kswenson added the v3 CODAP v3 label Oct 1, 2024
@eireland eireland marked this pull request as ready for review October 2, 2024 00:12
Copy link
Contributor

@tealefristoe tealefristoe left a comment

Choose a reason for hiding this comment

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

Overall this is looking good! I looked through much of the code and thought it was fine, then decided to test it. I think it generally works well, but I did have some problems with dragging to select. These all seem to be inconsistencies with v2. I'm not sure how many of these issues need to be addressed, but I think at least the final one should be, which is reproduceable.

  • Selection stops working when you drag outside of the collection table. In v2, selection continues to change as you move the mouse up and down anywhere in the document. If you then move the mouse back into the collection table, you can select discontinuous cases. I can imagine this one would be difficult to fix without a major overhaul of how this is set up.
  • Dragging up and down, I encountered issues with cases sometimes deselecting incorrectly. Maybe the most obvious example of this was starting in the middle of the table, dragging down, then dragging up above the starting case. Sometimes doing this, the starting case would deselect, while other times it wouldn't. In v2, the starting case never deselects when you scroll past it.
  • When you start dragging on a case that's already selected, it becomes deselected and never gets selected. In v2, if you start dragging on an already selected case, that case remains selected.
  • The final issue is that if you start dragging on the top case, then drag downwards, then back up to the top, the second case never becomes deselected. So in that case both the top and second case always remain selected.

Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Keyboard handling looks good. I didn't look too closely at the marquee selection code given @tealefristoe's comments. The traditional way of handling marquee selection is to identify the mouse-down location as the anchor, the current mouse position defines the other corner of a rectangle, and then you select the rows that intersect the rectangle. This avoids any need to track direction of motion and how many times the user may or may not have doubled back on themselves.

@@ -36,7 +36,7 @@ export function useWhiteSpaceClick({ gridRef }: IProps) {
})
}, [componentRef, data])

const handleWhiteSpaceClick = useCallback(() => {
const clearCurrentSelection = useCallback(() => {
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 I named this function handleWhiteSpaceClick originally is because a name like clearCurrentSelection implies rather strongly that calling it will clear the selection, but it doesn't actually change the selection under some very important circumstances. If you would like to have access to a function which actually clears the selection, then the clearCaseSelection function can be exported from this hook in addition to handleWhiteSpaceClick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 CODAP v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants