3.0 fixed columns and rows by Marc-Andre-Rivet · Pull Request #56 · plotly/dash-table

Marc-Andre-Rivet · 2018-09-04T17:57:30Z

This review is about making it possible to have both fixed columns and fixed rows at the same time -- previously, we could only use one at a time.

To achieve this, the DashTable is no longer a wrapper around a table element, rather, as an intermediary step in our quest to liberate ourselves from the table, it generates a two dimensional array of cells (still based on td and th) that are then sliced into multiple tables (yes, table is still there) that are then synchronized for styles, etc.
Rows are no longer a first-class citizen of the table, they are now just a render/implementation detail
basic sanity visual testing for fixed rows / columns scenarios

- duplicate component contract in TS

- bump version to rc6

- 3 tables for fixed rows/columns and content - update TS props

Marc-Andre-Rivet · 2018-09-04T18:02:13Z

dash_table/package.json

 {
  "name": "dash-table",
-  "version": "3.0.0rc5",
+  "version": "3.0.0rc6",


Bumping the version

Marc-Andre-Rivet · 2018-09-04T18:04:05Z

src/core/Clipboard.ts

+            ev.clipboardData.getData('text/plain') :
+            undefined;
+    }
+}


Wrapper around the clipboard implementation details (especially the copy portion) -- nothing here is dash or dash table specific

Marc-Andre-Rivet · 2018-09-04T18:04:31Z

src/core/math/random.ts

+    let safeSeed = seed !== undefined ? lcg(seed) : lcg(Math.random());
+
+    return () => (safeSeed = lcg(safeSeed)) / 2147483648;
+};


There's no seed in Math.random.. useful for testing purposes

Marc-Andre-Rivet · 2018-09-04T18:05:23Z

src/dash-table/Table.js

-                })}
-            />
-        );
+        return (<RealTable {...this.props} />);


This is the dash wrapper for the Dash Table.

Marc-Andre-Rivet · 2018-09-04T18:06:12Z

src/dash-table/components/CellFactory.tsx

+                    tableId={id}
+                    type={column.type}
+                    value={datum[column.id]}
+                />);


Refactored from the RowFactory, since there are no rows anymore.

Marc-Andre-Rivet · 2018-09-04T18:07:18Z

src/dash-table/components/HeaderCellFactory.tsx

-                    })}
-                </tr>
-            ));
+            return R.range(0, headerDepth).map(i => ([


Mostly the same but headers are not returned as a two dimensional array of headers instead of an array of rows

Marc-Andre-Rivet · 2018-09-04T18:08:17Z

src/dash-table/components/Table/VirtualizationAdapter.ts

    }

-    get virtualization(): string {
+    get virtualization(): Virtualization {


Just slowly improving the typing

Marc-Andre-Rivet · 2018-09-04T18:09:16Z

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

+
+        virtualizer.refresh();
+
+        return (<ControlledTable


This is the actual Table implementation used by the Dash wrapper --from this point on we are (in theory) in pure Typescript land

Marc-Andre-Rivet · 2018-09-04T18:10:57Z

src/dash-table/components/Table/props.ts

+    virtual_dataframe_indices: number[];
+    virtualization: Virtualization;
+    virtualization_settings: IVirtualizationSettings;
+}


PropsWithDefaults, IDefaultProps, IProps are the TS equivalent of the PropTypes impl in the table wrapper. Since the wrapper is responsible for the default props, there's no default props impl in the table itself -- still good to keep them divided.

Marc-Andre-Rivet · 2018-09-04T18:11:15Z

src/dash-table/components/derivedState.js


 export function colIsEditable(editable, column) {
-    return !(!editable || (R.has('editable', column) && !column.editable));
+    return editable && (!R.has('editable', column) || column.editable);


De Morgan and nested De Morgan..

Marc-Andre-Rivet · 2018-09-04T18:12:02Z

src/dash-table/utils/TableClipboardHelper.ts

+import { ActiveCell, Columns, Dataframe, SelectedCells } from 'dash-table/components/Table/props';
+
+export default class TableClipboardHelper {
+    public static toClipboard(selectedCells: SelectedCells, columns: Columns, dataframe: Dataframe) {


Dash table specific copy/paste implementation. Isolating the logic from the rest of the table impl

Marc-Andre-Rivet · 2018-09-04T18:12:45Z

tests/e2e/cypress/integration/copy_paste_test.ts


-        await Resolve(DashTable.getCell(1, 0).click());
-        await Resolve(DOM.focused.type(`${Key.Control}v`));
+    // it('can do BE roundtrip on copy-paste', () => {


Commenting but keep it as I am hopeful that they will resolve the issue above.

Marc-Andre-Rivet · 2018-09-04T18:14:02Z

tests/e2e/cypress/integration/navigation_test.ts

-    it('does not change column width', async () => {
-        const startWidth = await Resolve(DashTable.getCell(3, 3)).then(res => res.outerWidth());
+    it('does not change column width', () => {
+        DashTable.getCell(3, 3).then(startCell => {


the async/await + promise resolve impl was working well locally but for some reason it failed in CircleCI.. returning to the suggested way of doing things even-though it feels a bit archaic :)

Marc-Andre-Rivet · 2018-09-04T18:15:51Z

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

    }

-    switchCell(event) {
+    switchCell = (event: any) => {


Switching to class level lambda functions, no need to rebind this in ctor

Marc-Andre-Rivet · 2018-09-04T18:17:25Z

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

+            fixedColumnCells.splice(0, fixedRows) :
+            null;
+
+        return [


The matrix of all cells, slice through it for fixed rows and columns, make a sub-table with each fragment

Marc-Andre-Rivet · 2018-09-04T18:17:38Z

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

+    onScroll = (ev: any) => {
+        const { r0c1 } = this.refs as { [key: string]: HTMLElement };
+
+        r0c1.style.marginLeft = `${-ev.target.scrollLeft}`;


Keep the fixed rows synced with the main content

Marc-Andre-Rivet · 2018-09-04T18:19:43Z

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

+    }
+
+    componentDidUpdate() {
+        const { r0c0, r0c1, r1c0, r1c1 } = this.refs as { [key: string]: HTMLElement };


After render, make sure the styling between the various table-fragments is consistent -- mostly about row height now.

Marc-Andre-Rivet · 2018-09-04T18:21:09Z

@valentijnnieman @T4rk1n @bpostlethwaite Please have a look if you have some time. Thx.

Marc-Andre-Rivet · 2018-09-04T18:23:30Z

dash_table/package.json

+    "build:py": "./extract-meta src/dash-table/Table.js > dash_table/metadata.json && cp package.json dash_table",
    "lint": "run-s private::lint.js private::lint.ts",
-    "test": "run-p --race private::host* private::runtests private::snapshots",
+    "test": "run-s private::test-*",


Turns out the e2e tests and visual tests were racing one another -- visual tests are very short, so e2e tests were never run in CircleCI 😞

- fix scrolling issue in prod environment

Marc-Andre-Rivet · 2018-09-05T14:04:16Z

src/core/environment/index.ts

+        return log ?
+            (LogLevel as any)[log] || LogLevel.ERROR :
+            LogLevel.ERROR;
+    }


Making it possible to use cookies or additional params to override dash component behavior. In this case, toggling debug & log levels. Useful when testing against deployed component.

- additional visual tests for merged, fixed, hidden combo

Marc-Andre-Rivet · 2018-09-05T15:38:09Z

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

+        ];
+
+        // slice out fixed columns
+        const fixedColumnCells = fixedColumns ?


Take into consideration the "weight" of the merged cells when slicing the cells for the fixed columns

Marc-Andre-Rivet · 2018-09-05T15:49:31Z

src/core/Clipboard.ts

+        // Move outside the screen to make it invisible
+        // el.style.left = '-9999px';
+        // Append the <textarea> element to the HTML document
+        document.body.appendChild(el);


Copying this over / refactoring, leaving comments as is.

Marc-André Rivet and others added 30 commits August 29, 2018 09:30

- refactor: isolate dash component layer from component implementation

939e035

- duplicate component contract in TS

merge with master

6b38f3a

update for build js/py

24f8be7

- fix python component packaging regression

9b403c5

- bump version to rc6

Fix visual tests regression

81ee2c8

(wip) refactor controlled table

b40dc70

@types

3364c09

refactor table clipboard usage

8367945

clean up

81ad7b2

rename

1c0ff08

pr review fixes

1147ba6

fix bug in test script

e2439aa

fix test

ba37ad8

update test script

56e59fe

fix tests

6b3e1f6

fix tests

9651bcf

tests -- remove async/await syntax

f123fee

fix tests

0dfbd90

- remove row component

cc6bb94

- 3 tables for fixed rows/columns and content - update TS props

remove test code

c0994c9

fix test

ee98b20

remove test / broken by Cypress.io

79747bd

cypress version

35da207

fix cell selection test

6ba6193

fix percy tests

ffd0752

prevent useless cell updates once again

98d1a96

styling, copy/paste, navigation regressions

0b4481d

build, variables renamed

55c6228

fix next/previous regression

46e5f36

fix test regressions

1f48808

Marc-André Rivet added 2 commits September 4, 2018 13:09

fix test?

e84918b

revert percy removal

1309173

Marc-Andre-Rivet commented Sep 4, 2018

View reviewed changes

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

3.0 to multi sort or not #57

Closed

- add runtime logger configuration override

08b1607

- fix scrolling issue in prod environment

Marc-Andre-Rivet commented Sep 5, 2018

View reviewed changes

- fix merged cells / hidden cells w/ fixed col & rows

0a4accb

- additional visual tests for merged, fixed, hidden combo

Marc-Andre-Rivet commented Sep 5, 2018

View reviewed changes

Marc-Andre-Rivet merged commit 5b0d4fb into master Sep 5, 2018

Marc-Andre-Rivet deleted the 3.0-fixed-columns-and-rows branch September 27, 2018 15:15

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

styling examples #117

Merged

Uh oh!

Conversation

Marc-Andre-Rivet commented Sep 4, 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!

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!

Marc-Andre-Rivet Sep 4, 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!

Marc-Andre-Rivet Sep 4, 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!

Marc-Andre-Rivet commented Sep 4, 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!

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!

1 participant

Marc-Andre-Rivet commented Sep 4, 2018 •

edited

Loading

Marc-Andre-Rivet Sep 4, 2018 •

edited

Loading

Marc-Andre-Rivet Sep 4, 2018 •

edited

Loading

Marc-Andre-Rivet commented Sep 4, 2018 •

edited

Loading