Issue 649 - Fix mismatched row height for fixed columns #722

Marc-Andre-Rivet · 2020-03-17T16:19:24Z

Closes #649

This PR proposes to fix the misaligned rows issue by adding invisible cells in both the fixed columns and unfixed columns sections of the table. With some tweaks, this allows both fixed and unfixed cells to calculate their size based on the entire table's content, making sure that both size have the same size/height.

Afterwards the fixed rows can be coerced into having the correct width. With some extra tweaks for Firefox.

Percy browser versions have been updated for both dash-table and dash-table-python-v0 in order for the snapshots to match local behavior.

A lot of the changes in the PR is fixing the test logic to handle the added phantom-cell.

Current implementation slows down rendering for fixed table significantly (about 2.5x slower) but is significantly better than previous attempts (about 10-20x slower). Still investigating what can be done to reduce the overhead.

Possible secondary strategies:

Changing the default page_size from 250 to 100 would provide significant performance mitigation for default usage while not impacting usability significantly.

Marc-Andre-Rivet · 2020-03-23T21:15:25Z

demo/data.ts

+                    'console.log("logging things")',
+                    'console.warn("this is a markdown cell")'
+                ]
+            ),


Changing the mock to have variable height Markdown rows

Marc-Andre-Rivet · 2020-03-23T21:16:18Z

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

@@ -48,6 +48,9 @@ const INNER_STYLE = {
    minWidth: '100%'
 };

+const WIDTH_EPSILON = 0.5;
+const MAX_WIDTH_ITERATIONS = 30;


30 is a very high number but most of the time it takes 2-6 iterations for the table's column width to settle.

Marc-Andre-Rivet · 2020-03-23T21:16:42Z

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

@@ -146,7 +149,7 @@ export default class ControlledTable extends PureComponent<ControlledTableProps>

    componentDidUpdate() {
        this.updateStylesheet();
-        this.applyStyle();


applyStyle is removed / replaced by new code to handle the fixed rows

Marc-Andre-Rivet · 2020-03-23T21:17:24Z

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

+                    r0c1.style.marginLeft = `-${width2}`;
+                    r0c1.style.marginRight = `${width2}`;
+                    r1c1.style.marginLeft = `-${width2}`;
+                    r1c1.style.marginRight = `${width2}`;


If there are fixed columns, align the cell's table in such a way as to make the first visible column left-aligned with the cell.

Marc-Andre-Rivet · 2020-03-23T21:21:51Z

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

@@ -718,7 +700,10 @@ export default class ControlledTable extends PureComponent<ControlledTableProps>
        const { r0c1 } = this.refs as { [key: string]: HTMLElement };

        Logger.trace(`ControlledTable fragment scrolled to (left,top)=(${ev.target.scrollLeft},${ev.target.scrollTop})`);
-        r0c1.style.marginLeft = `${-ev.target.scrollLeft}px`;
+
+        const margin = parseFloat(ev.target.scrollLeft) + parseFloat(getComputedStyle(r0c1).marginRight);


margin-left and margin-right match in dimension, if not sign. Scroll adds to the initial offset.

Marc-Andre-Rivet · 2020-03-23T21:23:17Z

src/dash-table/components/Filter/Column.tsx

            columnId,
            isValid,
            style,
            value
        } = this.props;

        return (<th
-            className={classes + (isValid ? '' : ' invalid')}
+            className={className + (isValid ? '' : ' invalid')}


Standardizing prop name across all components simplifies manipulation elsewhere.

Marc-Andre-Rivet · 2020-03-23T21:24:04Z

src/dash-table/components/Table/Table.less

-		    color: graytext
-		}
-	    }
+        display: inline-block;


Indentation here was all weirded out - fixed. Lots of noise.

Looks like some got fixed and some other parts got mucked up. Mostly lines 507+ but there are a few before that that look weird as well. Is there an auto-formatter for .less?

OIC you have some hard tabs mixed in with spaces here - this file looks funny in #728 as well as a result.

I'll double check to make sure this file has been cleaned up.. but yes, at some point it became a mix of tabs/spaces

Marc-Andre-Rivet · 2020-03-23T21:26:25Z

src/dash-table/derived/table/fragments.tsx

+const getFirstPhantom = (cell: JSX.Element) => React.cloneElement(cell, {
+    ...cell.props,
+    style: { ...cell.props.style, borderLeft: 'transparent' }
+});


Various component cloning / manipulations.

getHiddenCell makes a phantom version of a cell - will be invisible
getFirstPhantom solves a doubled border issue

Marc-Andre-Rivet · 2020-03-23T21:26:56Z

tests/cypress/src/DashTable.ts

@@ -25,27 +25,35 @@ export class DashTableHelper {
    }

    public getCell(row: number, column: number, editable: State = State.Ready) {
-        return cy.get(`#${this.id} ${getSelector(editable)} tbody tr td.column-${column}`).eq(row);
+        return cy.get(`#${this.id} ${getSelector(editable)} tbody tr td.column-${column}:not(.phantom-cell)`).eq(row);


Update test helper to ignore phantom cells

Marc-Andre-Rivet · 2020-03-23T21:29:57Z

tests/visual/percy-storybook/MarkdownCells.percy.tsx

-            ]}
-            columns={columnFormats}
-        />))
+const variants: ITest[] = [


Markdown visual tests with both fixed an no fixed columns

Marc-Andre-Rivet · 2020-03-23T21:30:24Z

tests/visual/percy-storybook/Sizing.percy.tsx

        style_data_conditional={[{
+            if: { column_id: 'Region' },
+            whiteSpace: 'normal'
+        }]}


Use the standard prop to do this

Marc-Andre-Rivet · 2020-04-04T01:02:46Z

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

+
+            Array.from<HTMLElement>(
+                r0c1.querySelectorAll('table.cell-table > tbody > tr:last-of-type > *')
+            ).forEach((c, i) => this.setCellWidth(c, r1c1CellWidths[i]));


All this replaces and adds to the previous applyStyle method

alexcjohnson · 2020-04-13T22:35:55Z

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

+                ) {
+                    break;
+                }
+            } while (true);


Not a big deal, but given that ultimately there's a known limit (MAX_WIDTH_ITERATIONS) I'd have probably written this as a for loop, with only the width test down here breaking the loop early.

Want to keep both exit conditions together, that's why I opted for a while instead of a for loop. Did some clean up so the exit condition is now in the while condition instead of the body. 207c90a

Also removed the it % 2 == 0 check which is no longer necessary

OK, makes sense. Glad the it % 2 === 0 can be avoided!

alexcjohnson · 2020-04-13T23:09:49Z

src/dash-table/derived/table/fragments.tsx

@@ -1,6 +1,7 @@
 import * as R from 'ramda';
 import React from 'react';
 import { memoizeOneFactory } from 'core/memoizer';
+import isNil from 'ramda/es/isNil';


Why not just the one in import * as R from 'ramda'; above? In fact you use both copies...

Weird.. fixed with 33db6f3

alexcjohnson · 2020-04-13T23:18:33Z

src/dash-table/derived/table/fragments.tsx

+                },
+                { cells: 0, count: 0 },
+                row as any
+            ).cells;


🌴const pivot = ... looks identical to above?

🌴 3d0b8c8

alexcjohnson

Looks great! Just a few small comments, then this is ready to go! 💃

…able into 649-mismatched-row-heights

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 16:19 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 16:36 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 17:19 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 18:05 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 18:18 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 18:28 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 18:46 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 19:56 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 20:26 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 21:06 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 21:56 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 17, 2020 22:56 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 18, 2020 01:30 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 18, 2020 01:52 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 18, 2020 15:50 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 18, 2020 19:43 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 18, 2020 21:07 Inactive

chriddyp temporarily deployed to dash-table-review-pr-722 March 19, 2020 00:29 Inactive

chriddyp deployed to dash-table-review-pr-722 March 19, 2020 01:36 View deployment

Marc-Andre-Rivet commented Mar 23, 2020

View reviewed changes

Marc-André Rivet added 10 commits April 3, 2020 13:32

ignore phantom-cell

b9967ec

not a phantom cell

8523a76

undo

988ae85

move markdown keyboard navigation tests to selenium

37c5692

beforeEach..

26e6908

additional md visual tests with fixed rows/columns/both

144a616

add a row to md "table" visual test

5b5ca8c

only restyle the cell's table

f0bf0ea

fixed w/ deletable

f0d72aa

row_deletable

75bd7e6

Marc-Andre-Rivet commented Apr 4, 2020

View reviewed changes

Comment for cell width delta handling

9c043bd

Marc-Andre-Rivet changed the title ~~Fix mismatched row height for fixed columns~~ Issue 649 - Fix mismatched row height for fixed columns Apr 6, 2020

Update CHANGELOG.md

104ff53

alexcjohnson reviewed Apr 13, 2020

View reviewed changes

alexcjohnson approved these changes Apr 14, 2020

View reviewed changes

Marc-André Rivet added 6 commits April 14, 2020 10:02

Fix Ramda usage

33db6f3

Merge branch '649-mismatched-row-heights' of github.com:plotly/dash-t…

034db51

…able into 649-mismatched-row-heights

refactor duplicated code block

3d0b8c8

fix typing

59ff242

clean up exit condition for table fragments sizing

207c90a

fixed_rows horizontal alignment on scroll

99136c4

Marc-Andre-Rivet merged commit 60e0f91 into dev Apr 14, 2020

Marc-Andre-Rivet deleted the 649-mismatched-row-heights branch April 14, 2020 14:49

Marc-Andre-Rivet mentioned this pull request Apr 16, 2020

Expand column width to account for icons #730

Open

Marc-Andre-Rivet mentioned this pull request Jun 15, 2020

Issue 777 - Align column headers when using fixed rows #793

Merged

This was referenced Aug 19, 2020

Tooltip doesn't work with horizontal scroll #736

Closed

Issue 736 - Tooltip horizontal position incorrect #817

Merged

Uh oh!

Issue 649 - Fix mismatched row height for fixed columns #722

Issue 649 - Fix mismatched row height for fixed columns #722

Uh oh!

Conversation

Marc-Andre-Rivet commented Mar 17, 2020 • 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!

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!

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!

alexcjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Marc-Andre-Rivet commented Mar 17, 2020 •

edited

Loading