Add support for copying table cells with cmd+c hotkey #542

gscshoyru · 2017-01-25T18:40:24Z

Part of #298

Checklist

Enable CircleCI for your fork
Include tests
Update documentation

Changes proposed in this pull request:

You can specify a getCellData in the props and then
cmd+c will copy selected cells.

Reviewers should focus on:

What I'm doing here is sane, the shortcut only works when it's supposed to, etc.

giladgray

if you don't specify getCells, hotkey is still defined (will appear in dialog) but it'll never work.

giladgray · 2017-01-25T19:13:55Z

packages/table/src/table.tsx

+     * If exists, a callback that returns the data for a specific cell. This need not
+     * match the value displayed in the `<Cell>` component. The value will be
+     * invisibly added as `textContent` into the DOM before copying. If not exists,
+     * copy via hotkeys (mod+c) will not work


trailing period please

giladgray · 2017-01-25T19:14:34Z

packages/table/src/table.tsx

+        };
+
+        return <Hotkeys>
+            <Hotkey label="copy selected table cells" group="table" combo="mod+c" onKeyDown={doCopy} />


"Copy selected..." sentence casing

giladgray · 2017-01-25T19:14:56Z

packages/table/src/table.tsx

@@ -349,6 +360,30 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
        );
    }

+    public renderHotkeys() {
+        const doCopy = (e: KeyboardEvent) => {


why is this defined here instead of private handleCopyHotkey?

giladgray · 2017-01-25T19:16:05Z

packages/table/preview/index.tsx

+        return Utils.toBase26Alpha(col) + (row + 1);
+    };
+
+    const tablePropsWithDefaults = Object.assign({numRows, getCellData}, ...tableProps);


use the object spread notation that you just reverted. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-1.html#object-spread-and-rest

{ numRows, getCellData, ...tableProps }

@giladgray can you enable the ban lint rule to ban Object.assign?

giladgray · 2017-01-25T19:18:47Z

looks like it's time to increase table test coverage... can't keep lowering the threshold.

gscshoyru · 2017-01-25T19:19:25Z

That's fine I should add tests anyways; just wanted eyes on it first :)

leebyp · 2017-02-02T20:50:20Z

you can put the browser checks into the compatibility file introduced here #590

blueprint-bot · 2017-02-02T21:14:48Z

Check hypothesis about mod key keyboard events

Preview: docs | landing | table
Coverage: core | datetime

giladgray · 2017-02-03T01:28:50Z

packages/table/src/table.tsx

+                />
+            );
+        } else {
+            return null;


return undefined please

giladgray · 2017-02-03T01:31:26Z

packages/table/test/harness.ts

+}
+
+/**
+ * Use feature detection to determine current browser.


lol this is kinda backwards... you're supposed to use feature detection instead of browser detection: ensure that the feature you want exists, rather than asserting the browser "make." it's version-independent and much less prone to false positives.

is there a more reliable way of doing these checks that we can shim in here quickly?

at the very least move this to its own file.

I stole all of this from hotkeys test framework, that's how it does it. I couldn't figure out how to share test stuff between them. Is there a good way to do this? I'll totally do that.

And yeah agree that it sucks but...

adidahiya · 2017-02-03T06:21:22Z

packages/table/preview/index.tsx

+    };
+
+    const onCopy = () => {
+        console.log("did copy");


how about writing out the copied contents in this log?

Oooh better idea. 👍

Oh wait I can't do this because it's not passing that info in -- it just gets success. I'm not sure what I'd pass to it actually. Lemme think about it.

Maybe the on copy method itself should have an argument that contains the sparse copied cell data?

adidahiya · 2017-02-03T06:27:44Z

packages/table/src/table.tsx

+     * If exists, a callback that returns the data for a specific cell. This need not
+     * match the value displayed in the `<Cell>` component. The value will be
+     * invisibly added as `textContent` into the DOM before copying. If not exists,
+     * copy via hotkeys (mod+c) will not work.


The fact that this is used for mod+c copying should appear sooner in the description. also, friendlier language:

If provided, this callback will be used to get cell contents copied to a user's clipboard during a hotkey (mod+c) interaction in the <Cell> component. The value will be invisibly added as textContent into the DOM before copying.

I really wish we didn't need to make this prop required for the feature to be enabled (of course it should be configurable). Is there any way that users can get mod+c copying for free upon upgrading? Can we look at the rendered contents of their cell renderer?

This works the same as it does for the context menu copy -- you need to give that a callback too. The reason why is what you want rendered (with divs and styles and pretty things) may be radically different from what you want copied (the data). I've talked to @themadcreator at length about this, and this seems like the best way :-/

For example, @adidahiya, if they select a whole column, we'd want to copy all cells in the columns, but due to virtualized rendering, there's no guarantee that all the cells in that column will have ever been rendered. Furthermore, some cells have truncated content (like JSON content), so copying the actual cell text would be incomplete.

But we could consider renaming this getCellClipboardData since without the naming, users might think this is used to fill cells when rendering.

giladgray · 2017-02-03T19:39:36Z

packages/table/src/table.tsx

+        if (sparse != null) {
+            const success = Clipboard.copyCells(sparse);
+            if (onCopy != null) {
+                onCopy(success);


use Utils.safeInvoke from the core package for this pattern please

What is this for? I can see that it exists; I just don't understand why.

safely invokes a maybe-defined callback. it's precisely this pattern (if function exists then call it) expressed as one type-safe statement.

It's to make the classic "check null, then invoke" code go from 3 lines to one.

giladgray · 2017-02-03T19:39:51Z

packages/table/src/table.tsx

@@ -660,6 +711,22 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
        });
    }

+    private maybeRenderCopyHotkey() {
+        const {getCellData} = this.props;


spaces in braces

giladgray · 2017-02-03T19:40:11Z

packages/table/src/table.tsx

+                />
+            );
+        } else {
+            return null;


Part of #298 . You can specify a getCellData in the props and then this will work. Let me know if you all want to go about this differently.

themadcreator

Want to consider renaming the get data method

themadcreator · 2017-02-03T20:14:44Z

packages/table/src/table.tsx

+     * If exists, a callback that returns the data for a specific cell. This need not
+     * match the value displayed in the `<Cell>` component. The value will be
+     * invisibly added as `textContent` into the DOM before copying. If not exists,
+     * copy via hotkeys (mod+c) will not work.


But we could consider renaming this getCellClipboardData since without the naming, users might think this is used to fill cells when rendering.

themadcreator · 2017-02-03T20:16:50Z

packages/table/src/table.tsx

+        if (sparse != null) {
+            const success = Clipboard.copyCells(sparse);
+            if (onCopy != null) {
+                onCopy(success);


It's to make the classic "check null, then invoke" code go from 3 lines to one.

themadcreator · 2017-02-03T20:17:58Z

packages/table/src/table.tsx

+            return (
+                <Hotkey
+                    label="Copy selected table cells"
+                    group="table"


Should group be capitalized? You can see the result my pressing shift + ? when the table is focused:

themadcreator · 2017-02-03T20:18:32Z

packages/table/test/harness.ts

+
+    (event as any).initKeyboardEvent(eventType, true, true, window, key, 0, ctrlKey, false, false, metaKey);
+
+    // Hack around these readonly properties in WebKit and Chrome


I'm very sorry =(

themadcreator · 2017-02-03T20:20:25Z

packages/table/test/selectionTests.tsx

+        table.find(TH_SELECTOR).focus();
+        table.find(TH_SELECTOR).keyboard("keydown", "C", true);
+        expect(copyCellsStub.lastCall.args).to.deep.equal([[["A1"], ["A2"], ["A3"], ["A4"], ["A5"], ["A6"], ["A7"]]]);
+        expect(onCopy.lastCall.args).to.deep.equal([true]);


themadcreator · 2017-02-03T21:27:05Z

Okay, my requests have been addressed, though build is red

giladgray · 2017-02-03T21:33:53Z

packages/core/src/compatibility/browser.ts

@@ -9,9 +9,11 @@ const userAgent = typeof navigator !== "undefined" ? navigator.userAgent : "";
 const browser = {
    isEdge: !!userAgent.match(/Edge/),
    isInternetExplorer: (!!userAgent.match(/Trident/) || !!userAgent.match(/rv:11/)),
+    isWebkit: Object.prototype.toString.call((window as any).HTMLElement).indexOf("Constructor") > 0,


probably >= 0?

also make sure window is defined first

giladgray · 2017-02-03T21:48:14Z

packages/core/src/compatibility/browser.ts

@@ -9,9 +9,12 @@ const userAgent = typeof navigator !== "undefined" ? navigator.userAgent : "";
 const browser = {
    isEdge: !!userAgent.match(/Edge/),
    isInternetExplorer: (!!userAgent.match(/Trident/) || !!userAgent.match(/rv:11/)),
+    isWebkit: !!userAgent.match(/AppleWebKit/),
+    // isWebkit: Object.prototype.toString.call((window as any).HTMLElement).indexOf("Constructor") > 0,


please remove commented code

blueprint-bot · 2017-02-03T21:50:27Z

Better user agent check

Preview: docs | landing | table
Coverage: core | datetime

blueprint-bot · 2017-02-03T21:55:45Z

Remove commented code (whoops)

Preview: docs | landing | table
Coverage: core | datetime

gscshoyru force-pushed the gsc/add-copy-on-shortcuts branch from 003d4e0 to 171ee5a Compare January 25, 2017 18:50

adidahiya changed the title ~~Add option for letting cmd+c copy~~ Support copying table cells with cmd+c hotkey Jan 25, 2017

adidahiya changed the title ~~Support copying table cells with cmd+c hotkey~~ Add support for copying table cells with cmd+c hotkey Jan 25, 2017

giladgray suggested changes Jan 25, 2017

View reviewed changes

llorca requested a review from themadcreator January 25, 2017 20:04

giladgray reviewed Feb 3, 2017

View reviewed changes

adidahiya reviewed Feb 3, 2017

View reviewed changes

giladgray added the P0 label Feb 3, 2017

giladgray suggested changes Feb 3, 2017

View reviewed changes

gscshoyru added 3 commits February 3, 2017 15:03

Add option for letting cmd+c copy

4f5dd48

Part of #298 . You can specify a getCellData in the props and then this will work. Let me know if you all want to go about this differently.

Address comments, add test and test framework

5383090

Check hypothesis about mod key keyboard events

e375098

themadcreator suggested changes Feb 3, 2017

View reviewed changes

Address more comments

5bd6c37

gscshoyru force-pushed the gsc/add-copy-on-shortcuts branch from eac003e to 5bd6c37 Compare February 3, 2017 21:20

themadcreator approved these changes Feb 3, 2017

View reviewed changes

giladgray reviewed Feb 3, 2017

View reviewed changes

Better user agent check

2f88f83

gscshoyru force-pushed the gsc/add-copy-on-shortcuts branch from 1aec1cf to 2f88f83 Compare February 3, 2017 21:45

giladgray reviewed Feb 3, 2017

View reviewed changes

giladgray approved these changes Feb 3, 2017

View reviewed changes

Remove commented code (whoops)

265d2ca

giladgray merged commit b019cde into master Feb 3, 2017

giladgray deleted the gsc/add-copy-on-shortcuts branch February 3, 2017 22:10

giladgray removed the P0 label Feb 3, 2017

giladgray mentioned this pull request Feb 3, 2017

Prepare Release v1.8.0 #616

Merged


		(event as any).initKeyboardEvent(eventType, true, true, window, key, 0, ctrlKey, false, false, metaKey);

		// Hack around these readonly properties in WebKit and Chrome

Add support for copying table cells with cmd+c hotkey #542

Add support for copying table cells with cmd+c hotkey #542

Conversation

gscshoyru commented Jan 25, 2017 • edited Loading

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

giladgray commented Jan 25, 2017

gscshoyru commented Jan 25, 2017

leebyp commented Feb 2, 2017

blueprint-bot commented Feb 2, 2017

Check hypothesis about mod key keyboard events

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themadcreator Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

adidahiya Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themadcreator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themadcreator commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Feb 3, 2017

Better user agent check

blueprint-bot commented Feb 3, 2017

Remove commented code (whoops)

gscshoyru commented Jan 25, 2017 •

edited

Loading

adidahiya Jan 25, 2017 •

edited

Loading

themadcreator Feb 3, 2017 •

edited

Loading

adidahiya Feb 3, 2017 •

edited

Loading