-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: multiselect values #1736
feat: multiselect values #1736
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1736 +/- ##
==========================================
- Coverage 46.06% 45.99% -0.08%
==========================================
Files 626 626
Lines 37596 37716 +120
Branches 9460 9493 +33
==========================================
+ Hits 17319 17346 +27
- Misses 20222 20315 +93
Partials 55 55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
1ab8a7d
to
cf80cf8
Compare
I'll want to review the right click behaviour changes when available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equals/not equals should be ORs and contains/starts with/ends with should be ANDs
- I know I said this in the ticket. I tried it and I was clearly wrong. It does not make sense for strings. It should really be just ORs for all string comparisons types.
- If you right click outside of the selection it should actually be changing the selection to the row you right-clicked on first.
Right-clicking on a row that isn’t selected should immediately change the selection to that row and present the menu options for that row
like this:
I see what you did with the greater than/less than for numbers. I like that.
It's a little confusing at first if your set contains infinity, but I guess that's reasonable as it can't have greater than or less. Wonder if there's some sort of user feedback we should be giving though. Thoughts?
How about if positive infinity is present, then > and >= are disabled, and the similarly for positive infinity? |
|
Yes, that is what is presently happening. However, I am saying It can be confusing. For example, say you select 500 rows. On screen you see 1, 2, 3, 4 etc. but somewhere in that selection set is -infinity. You don't know it's there you only know that for some reason you can only select equals/not equals and it's unclear as to why. Maybe we should treat it like null and exclude it from the set if selection an option other than equals not equals? |
Since the For example, if the selection was 1 and infinity:
If there's a NaN, then filter can also be disabled with a "Selection includes NaN" tooltip. |
58ff5fd
to
d397a4f
Compare
Tooltips in a context menu seems like something one shouldn't do. I am gonna recommend dropping it from the filter for cases other than equals/not equals, as that would probably behave inline with a users intent of selecting less than/greater than. |
@@ -355,6 +355,7 @@ class Grid extends PureComponent<GridProps, GridState> { | |||
this.handleMouseUp = this.handleMouseUp.bind(this); | |||
this.handleResize = this.handleResize.bind(this); | |||
this.handleWheel = this.handleWheel.bind(this); | |||
this.getSelectedRanges = this.getSelectedRanges.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binding this here is not strictly necessary, as getSelectedRanges
isn't passed in anywhere. Should just be able to call grid.getSelectedRanges
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting TypeError: Cannot read properties of undefined (reading 'state')
when I'm not binding it. I believe it's because the function is reading selectedRanges
from this.state
.
grid.clearSelectedRanges(); | ||
grid.moveSelection(gridPoint.column, gridPoint.row); | ||
grid.commitSelection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just call moveCursorToPosition
instead:
grid.clearSelectedRanges(); | |
grid.moveSelection(gridPoint.column, gridPoint.row); | |
grid.commitSelection(); | |
grid.moveCursorToPosition(gridPoint.column, gridPoint.row); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced
grid.moveSelection(gridPoint.column, gridPoint.row);
grid.commitSelection();
with
grid.moveCursorToPosition(gridPoint.column, gridPoint.row);
It seems like clearSelectedRanges
is needed, otherwise the the right click will just add to the selection.
packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx
Outdated
Show resolved
Hide resolved
packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx
Outdated
Show resolved
Hide resolved
packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Mike Bender <mikebender@deephaven.io>
Co-authored-by: Mike Bender <mikebender@deephaven.io>
Co-authored-by: Mike Bender <mikebender@deephaven.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple suggestions for optimizations/refactors.
packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx
Outdated
Show resolved
Hide resolved
packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
async
function that gets a snapshot to supports multiple valuesMAX_MULTISELECT_ROWS
) rows will be snapshotted for unique values. The reason why its rows and not unique values is to prevent the case of a very large amount of rows with a very small amount of unique values.