fix regression + basic test by Marc-Andre-Rivet · Pull Request #70 · plotly/dash-table

Marc-Andre-Rivet · 2018-09-11T02:07:39Z

This review is pretty big and divided in two parts

the actual fix for the copy/paste issue + the delete cell issue + save on click out/blur issue (not much here -- mostly adding transforms between the real dataframe and the virtualized one and offsets)
making it possible to test the copy/paste scenario in CI tests -- this requires overriding some behavior so that the copy/paste behaves in a way compatible with cypress -- this means creating a custom test configuration / special test code that overrides the normal behavior of the copy/paste while staying as close as possible to the original code so the test is still significant. ......... This is of course a bit hacky but well worth the price IMO.. this is the 2nd regression on this feature in 2 weeks, I'll take whatever flavor of coverage I can get!
adding tests for the regressions

So, the special test version will handle the paste event through the onKeyDown event and call the onPaste handler. Copy will keep the copied value cached as it will be required -- there's no onPaste event -- and that's the only place where the clipboard data is made available.

Marc-Andre-Rivet · 2018-09-11T02:08:25Z

dash_table/package.json

    "preprivate::opentests": "run-s private::wait*",
    "preprivate::runtests": "run-s private::wait*",
    "private::build": "webpack --display-reasons --bail",
+    "private::build_test": "webpack --display-reasons --bail --config webpack.test.config.js",


A special 'test' build with a slightly different configuration for webpack

Marc-Andre-Rivet · 2018-09-11T02:11:15Z

src/core/Clipboard.ts

 export default class Clipboard {
+    /*#if TEST_COPY_PASTE*/
+    private static value: string;
+    /*#endif*/


Webpack-preprocessor allows for conditional/build based code. This sometimes makes for pretty involved code.. but is a lot more flexible than, say, having an alias to some folder in dev vs test vs prod.

Have to be careful though.. for the editor there is no if..else.. so all the code co-exists

Marc-Andre-Rivet · 2018-09-11T02:13:15Z

src/core/Clipboard.ts

+        let value;
+
+        /*#if TEST_COPY_PASTE*/
+        value = Clipboard.value;


Keep the copy value in a variable.. there's no event clipboard value in the test environment.. return the stored value instead!

Marc-Andre-Rivet · 2018-09-11T02:13:55Z

src/dash-table/components/ControlledTable/index.tsx

        // if paste event onPaste handler registered in Table jsx handles it
        if (ctrlDown && e.keyCode === KEY_CODES.V) {
+            /*#if TEST_COPY_PASTE*/
+            this.onPaste({} as any);


Normally the paste event is handled on its own, but here we need to call it from onkeydown instead

Marc-Andre-Rivet · 2018-09-11T02:14:43Z

src/dash-table/components/ControlledTable/index.tsx

+            (row_deletable ? 1 : 0) +
+            (row_selectable ? 1 : 0);
+
+        const noOffsetSelectedCells: [number, number][] = R.map(


Actual offset fix. These offsets are creeping up everywhere.. once we have coverage we should refactor them out, they are useless and complicate everything.

Agreed, this is kind of confusing and perhaps a culprit for regressions. Not especially a fan of this const columnIndexOffset being repeated in switchCell, deleteCell, getNextCell, onCopy, onPaste etc..

Marc-Andre-Rivet · 2018-09-11T02:16:35Z

tests/e2e/cypress/plugins/index.js

 module.exports = (on) => {
    const options = {
-        webpackOptions: require('../../../../webpack.config'),
+        webpackOptions: require('../../../../webpack.test.config.js'),


Cypress uses the test configuration

Marc-Andre-Rivet · 2018-09-11T02:16:59Z

webpack.test.config.js

+    {
+        definitions: ['TEST_COPY_PASTE']
+    },
+    'development'


preprocessor and mode overrides

Marc-Andre-Rivet · 2018-09-11T12:40:51Z

.circleci/config.yml

            - run:
                name: Run build:js
-                command: npm run build:js
+                command: npm run build:js-test


Build the test version..

cldougl · 2018-09-11T13:04:18Z

tests/e2e/cypress/integration/copy_paste_test.ts

    });

-    // Commenting this test as Cypress team is having issues with the copy/paste scenario
-    // https://github.com/cypress-io/cypress/issues/2386


@Marc-Andre-Rivet maybe we want to add a TODO and keep a link to this issue in the code so we can rework this test if/when the above issue is resolved.

@cldougl Good point. I'll add it back.

cldougl · 2018-09-11T13:25:38Z

@Marc-Andre-Rivet copy and pasting works well for me initially, but it does not work if I sort a column:

lmk if I can provide more information

Marc-Andre-Rivet · 2018-09-11T13:27:42Z

@cldougl Thanks, will test it out.. guessing that if you remove the sorting, the data has been copied at the beginning of the dataframe -- behavior confirmed

Marc-Andre-Rivet · 2018-09-11T14:10:37Z

@cldougl This problem sort/filter issue will also apply to delete cell #67 but should not affect actual cell editing

- delete cell with and w/o sorting - additional tests for copy/paste

Marc-Andre-Rivet · 2018-09-11T14:48:19Z

CHANGELOG.md

+## RC11 - Fix copy/paste regression, fix delete regression
+
+    Issue: https://github.com/plotly/dash-table/issues/64
+    Issue: https://github.com/plotly/dash-table/issues/67


Ended up working on both celle delete and copy/paste as it's the same issue

Marc-Andre-Rivet · 2018-09-11T14:49:47Z

src/dash-table/components/ControlledTable/index.tsx

+            selected_cell
+        );
+
+        realCells.forEach(cell => {


Since we need to work on the real dataframe and the viewport is showing a virtualized subsection of the whole df, we need to map between the virtualized df and the real one to modify the correct indices.

Marc-Andre-Rivet · 2018-09-11T14:50:57Z

src/dash-table/utils/TableClipboardHelper.ts

+        }
+
+        const realActiveRow = virtual_dataframe_indices[activeCell[0]];
+        if (overflowRows && values.length + realActiveRow >= dataframe.length) {


We normally add rows if the copy/paste requires more entries, but with sorting or filtering on, it's not obvious what the behavior should be. Preventing this from happening for now.

Marc-Andre-Rivet · 2018-09-11T14:51:16Z

src/dash-table/utils/TableClipboardHelper.ts

                // let newDataframe = dataframe;
                const col = newColumns[jOffset];
-                if (colIsEditable(true, col)) {
+                if (col && colIsEditable(true, col)) {


It's possible now that the column was not created, checking for it

Marc-Andre-Rivet · 2018-09-11T14:51:37Z

src/dash-table/utils/TableClipboardHelper.ts

+            row.forEach((cell: string, j: number) => {
                const iOffset = activeCell[0] + i;
+                if (virtual_dataframe_indices.length <= activeCell[0] + i) {
+                    return;


row may not exist bc of the overflow toggle

Marc-Andre-Rivet · 2018-09-11T14:51:57Z

tests/e2e/cypress/integration/copy_paste_test.ts

+                .within(() => cy.get('.cell-value').should('have.value', 'MODIFIED'));
+        });
+
+        it('BE rountrip with sorted, unfiltered data', () => {


new test case for sorted data + copy/paste

cldougl · 2018-09-11T16:26:35Z

💃 this looks a lot better to me!
As this is bug/regression fixes and test additions (great job adding tests @Marc-Andre-Rivet 🎉 🎉 )
I'm happy if we can merge this now to generate a bundle asap as this fixes a lot of issues.
@wbrgss feel free to continue to reviewing post merge if you have some comments or any questions about this 'workaround' testing structure!

wbrgss

Tests look good to me. Too bad about having to hack around the Cypress issue but I agree better there than not! 💃

wbrgss · 2018-09-11T16:19:46Z

src/dash-table/components/ControlledTable/index.tsx

+            (row_deletable ? 1 : 0) +
+            (row_selectable ? 1 : 0);
+
+        const realCells: [number, number][] = R.map(


Performance consideration: might be worth checking alternatives to R.map for mapping between many virtual => real cells if this is executing a lot. Same goes for noOffsetSelectedCells. Benchmark (higher is better):
https://www.measurethat.net/Benchmarks/ShowResult/32960

I know Ramda use is encouraged, but if you're looking to squeeze out some better UX responsiveness..

@wbrgss Agreed. Just being consistent. All this needs to be reworked. I have a placeholder issue #71 -- it feels like the virtualization layer will be the right place to put this in, make consistent, and optimize.

wbrgss · 2018-09-11T16:28:52Z

src/dash-table/components/ControlledTable/index.tsx

+            (row_deletable ? 1 : 0) +
+            (row_selectable ? 1 : 0);
+
+        const noOffsetSelectedCells: [number, number][] = R.map(


Agreed, this is kind of confusing and perhaps a culprit for regressions. Not especially a fan of this const columnIndexOffset being repeated in switchCell, deleteCell, getNextCell, onCopy, onPaste etc..

fix regression + basic test

0e7bd78

Marc-Andre-Rivet commented Sep 11, 2018

View reviewed changes

build.watch, not test

3bf6f77

Marc-Andre-Rivet commented Sep 11, 2018

View reviewed changes

remove .only on tests...

c6f556a

Marc-Andre-Rivet commented Sep 11, 2018

View reviewed changes

Marc-Andre-Rivet requested review from bpostlethwaite and wbrgss September 11, 2018 02:17

Marc-Andre-Rivet mentioned this pull request Sep 11, 2018

copy and paste is shifted to the right #64

Closed

fix copy test

8fb0f7f

Marc-Andre-Rivet commented Sep 11, 2018

View reviewed changes

cldougl reviewed Sep 11, 2018

View reviewed changes

add back cypress issue comment

31fe33f

Marc-André Rivet added 2 commits September 11, 2018 10:29

- copy/paste with sorting

133f568

- delete cell with and w/o sorting - additional tests for copy/paste

- delete cell tests for sorted and sorted df

24d96c4

Marc-Andre-Rivet commented Sep 11, 2018

View reviewed changes

Marc-Andre-Rivet mentioned this pull request Sep 11, 2018

delete action shifted to right #67

Closed

fix saving changes on click outside of cell

d0757e9

Marc-Andre-Rivet mentioned this pull request Sep 11, 2018

clicking out of cell doesn't save edits #65

Closed

Marc-André Rivet added 2 commits September 11, 2018 11:18

update changelog

242cf0c

fix useless propagates on click outside / blur

edd30cd

wbrgss approved these changes Sep 11, 2018

View reviewed changes

Marc-Andre-Rivet merged commit 05ced2e into master Sep 11, 2018

Marc-Andre-Rivet deleted the 3.0-copy-paste-regression branch September 12, 2018 11:31

Marc-Andre-Rivet mentioned this pull request Oct 22, 2018

Clean up column offset usage #71

Closed

Uh oh!

Conversation

Marc-Andre-Rivet commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cldougl commented Sep 11, 2018

Uh oh!

Marc-Andre-Rivet commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marc-Andre-Rivet commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cldougl commented Sep 11, 2018

Uh oh!

wbrgss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Marc-Andre-Rivet commented Sep 11, 2018 •

edited

Loading

Marc-Andre-Rivet Sep 11, 2018 •

edited

Loading

Marc-Andre-Rivet commented Sep 11, 2018 •

edited

Loading