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

feat: Add alt+click shortcut to copy cell and column headers #1694

Merged
merged 6 commits into from
Dec 22, 2023

Conversation

georgecwan
Copy link
Contributor

Adds a alt/opt+click shortcut to copy the value of a cell or column header.

Resolves #1585.

@georgecwan georgecwan self-assigned this Dec 14, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (9d519e1) 46.74% compared to head (1eb45ec) 46.65%.
Report is 2 commits behind head on main.

Files Patch % Lines
.../src/mousehandlers/IrisGridCopyCellMouseHandler.ts 15.38% 22 Missing ⚠️
packages/iris-grid/src/IrisGrid.tsx 23.52% 13 Missing ⚠️
packages/iris-grid/src/IrisGridCopyHandler.tsx 83.33% 7 Missing ⚠️
packages/grid/src/Grid.tsx 55.55% 4 Missing ⚠️
...d/src/mousehandlers/IrisGridContextMenuHandler.tsx 0.00% 4 Missing ⚠️
packages/grid/src/KeyHandler.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1694      +/-   ##
==========================================
- Coverage   46.74%   46.65%   -0.09%     
==========================================
  Files         606      607       +1     
  Lines       36762    36945     +183     
  Branches     9227     9294      +67     
==========================================
+ Hits        17184    17238      +54     
- Misses      19526    19655     +129     
  Partials       52       52              
Flag Coverage Δ
unit 46.65% <48.51%> (-0.09%) ⬇️

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.

@georgecwan georgecwan marked this pull request as ready for review December 14, 2023 19:06
@georgecwan georgecwan force-pushed the 1585-alt-click-shortcut branch from 8b6201a to 02aaa14 Compare December 15, 2023 21:44
@georgecwan georgecwan force-pushed the 1585-alt-click-shortcut branch from 02aaa14 to 4e43d37 Compare December 15, 2023 21:49
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Good start! Couple of things we can improve.

@@ -632,6 +633,9 @@ class IrisGridContextMenuHandler extends GridMouseHandler {
actions.push({
title: 'Copy Cell',
group: IrisGridContextMenuHandler.GROUP_COPY,
shortcutText: ContextActionUtils.isMacPlatform()
Copy link
Member

Choose a reason for hiding this comment

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

I was debating if we should add support for Shortcut to handle mouse clicks... would be kind of interesting in that they could be configurable, but it's more of a headache than we need right now.

packages/grid/src/Grid.tsx Outdated Show resolved Hide resolved
@mofojed
Copy link
Member

mofojed commented Dec 21, 2023

@georgecwan you can test with copy permissions disabled by setting the permission flag on startup, e.g.:

START_OPTS="-Dinternal.webClient.appInit.canCopy=false" ./gradlew server-jetty-app:run -Pdebug -Ppsk=iris

@georgecwan georgecwan requested a review from mofojed December 22, 2023 00:11
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Something is strange with the snapshot function in general. When I'm trying to copy 100,000 rows, it's only copying 50,000. Table I'm using is:

from deephaven import empty_table
t = empty_table(100000).update(["X=i", "Y=i*i"])

I don't think related to your change though, seems to be in the JS API. Interesting. Logged it with ticket https://github.com/deephaven/web-client-ui/issues/1703 for further investigation.

Other than the minor handleKeyDown cleanup on Grid, should also write a unit test in IrisGridCopyHandler.test.tsx for the copy header cases.

packages/grid/src/Grid.tsx Outdated Show resolved Hide resolved
@georgecwan georgecwan merged commit 4a8a81a into deephaven:main Dec 22, 2023
4 of 5 checks passed
@georgecwan georgecwan deleted the 1585-alt-click-shortcut branch December 22, 2023 18:56
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
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.

Shortcut to copy value from an individual cell with alt/opt + click
3 participants