Skip to content

feat: Add support to pass in mouseHandlers into IrisGrid#1857

Merged
mofojed merged 2 commits intodeephaven:mainfrom
mofojed:iris-grid-mouse-handlers
Mar 8, 2024
Merged

feat: Add support to pass in mouseHandlers into IrisGrid#1857
mofojed merged 2 commits intodeephaven:mainfrom
mofojed:iris-grid-mouse-handlers

Conversation

@mofojed
Copy link
Copy Markdown
Member

@mofojed mofojed commented Mar 7, 2024

- Move the doubleClick behaviour from IrisGridDataSelectMouseHandler into GridSelectionMouseHandler, so it's baked into Grid itself (double-clicking a grid in the styleguide was exhibiting erroneous behaviour)
- Allow passing in mouseHandlers to IrisGrid. Will be necessary to support deephaven/deephaven-plugins#320
- Tested using my branch for on_row_press support on deephaven.ui
- Verified Linker still worked as expected
@mofojed mofojed requested a review from bmingles March 7, 2024 21:41
@mofojed mofojed self-assigned this Mar 7, 2024
mofojed added a commit to mofojed/deephaven-plugins that referenced this pull request Mar 7, 2024
- Depends on my web PR: deephaven/web-client-ui#1857
- Passes through the on_*_press events to IrisGrid
- Tested with the snippet from the examples, ensured all events were being printed with the correct information:
```python
import deephaven.ui as ui
import deephaven.plot.express as dx

te = (
    ui.table(dx.data.stocks())
    .on_row_press(lambda row, data: print(f"Row Press: {row}, {data}"))
    .on_row_double_press(lambda row, data: print(f"Row Double Press: {row}, {data}"))
    .on_cell_press(
        lambda cell_index, data: print(f"Cell Press: {cell_index}, {data}")
    )
    .on_cell_double_press(
        lambda cell_index, data: print(f"Cell Double Press: {cell_index}, {data}")
    )
    .on_column_press(lambda column: print(f"Column Press: {column}"))
    .on_column_double_press(
        lambda column: print(f"Column Double Press: {column}")
    )
)
```
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 46.11%. Comparing base (1ce6aac) to head (767f5a3).
Report is 3 commits behind head on main.

Files Patch % Lines
...id/src/mouse-handlers/GridSelectionMouseHandler.ts 0.00% 8 Missing ⚠️
packages/iris-grid/src/IrisGrid.tsx 72.22% 5 Missing ⚠️
...rc/mousehandlers/IrisGridDataSelectMouseHandler.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1857      +/-   ##
==========================================
- Coverage   46.12%   46.11%   -0.02%     
==========================================
  Files         635      636       +1     
  Lines       38025    38070      +45     
  Branches     9612     9627      +15     
==========================================
+ Hits        17540    17556      +16     
- Misses      20432    20461      +29     
  Partials       53       53              
Flag Coverage Δ
unit 46.11% <50.00%> (-0.02%) ⬇️

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.

getCachedMouseHandlers = memoize(
(
mouseHandlers: readonly GridMouseHandler[]
): readonly GridMouseHandler[] => [...mouseHandlers, ...this.mouseHandlers]
Copy link
Copy Markdown
Contributor

@bmingles bmingles Mar 7, 2024

Choose a reason for hiding this comment

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

It looks like this.mouseHandlers gets set by initial props and then any updates to the props get combined with the initial. Am I understanding this correctly, and if so, is that what we want, or should the initial props be dropped if props change? Same question for getCachedKeyHandlers

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this.mouseHandlers and this.keyHandlers are not getting by initial props, that's a local value that is initialized in the constructor.
Then the getCachedMouseHandlers combines the default/code IrisGrid mouseHandlers with the custom provided mouseHandlers provided from the props.
We use a similar pattern in Grid.tsx as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, misread. Looks good then

Comment thread packages/iris-grid/src/mousehandlers/IrisGridDataSelectMouseHandler.ts Outdated
Copy link
Copy Markdown
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Left a question about event handlers. Also looks like a kerning e2e test failure.

@mofojed mofojed requested a review from bmingles March 8, 2024 16:57
bmingles
bmingles previously approved these changes Mar 8, 2024
Copy link
Copy Markdown
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM

@mofojed mofojed merged commit acf32a6 into deephaven:main Mar 8, 2024
@mofojed mofojed deleted the iris-grid-mouse-handlers branch March 8, 2024 20:36
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants