[Table] Pass parent width/height props to TruncatedFormat #1550

gscshoyru · 2017-09-11T21:45:53Z

Changes proposed in this pull request:

Pass cell height and width to child, for use in shouldComponentUpdate

~~The props aren't used by the element, but its presence means faster redraw for cells with truncated formats.~~ The props are used by the element to avoid re-measuring if the parent size didn't change across updates.

Reviewers should focus on:

This is on top of another in-progress PR -- ignore the first two commits.

I'm cloning all the cell children -- does this make sense?

cmslewis · 2017-09-11T21:48:45Z

packages/table/src/cell/cell.tsx

+        const modifiedChildren = React.Children.map(this.props.children, (child) => {
+            if (React.isValidElement(child)) {
+                return React.cloneElement(child as React.ReactElement<any>,
+                    {parentCellHeight: style.height, parentCellWidth: style.width});


nit: add spaces: { parentCellHeight: style.height, parentCellWidth: style.width }

cmslewis · 2017-09-11T21:49:38Z

packages/table/src/cell/cell.tsx

@@ -102,7 +102,15 @@ export class Cell extends React.Component<ICellProps, {}> {
            },
        );

-        const content = <div className={textClasses}>{this.props.children}</div>;
+        const modifiedChildren = React.Children.map(this.props.children, (child) => {


add comment somewhere explaining that parentCell{Height,Width} won't be used in the Cell except in shouldComponentUpdate

cmslewis · 2017-09-11T21:49:59Z

packages/table/src/cell/formats/truncatedFormat.tsx

+            || actualContentWidth > containerWidth
+            || actualContentHeight > containerHeight;
+
+        if (isTruncated !== shouldTruncate) {


Still need this?

@gscshoyru I think we can do away with this check now that this is a pure component.

blueprint-bot · 2017-09-11T22:26:26Z

Check for null to fix tests

Preview: documentation | table
Coverage: core | datetime

tgreenwatts · 2017-09-11T22:29:31Z

Do we know yet if this helps perf? Or just an exercise still?

giladgray · 2017-09-12T18:43:50Z

packages/table/src/cell/formats/truncatedFormat.tsx

@@ -132,6 +136,11 @@ export class TruncatedFormat extends React.Component<ITruncatedFormatProps, ITru
        this.setTruncationState();
    }

+    public shouldComponentUpdate(nextProps: ITruncatedFormatProps, nextState: ITruncatedFormatState) {
+        return !Utils.shallowCompareKeys(this.props, nextProps)
+            || !Utils.shallowCompareKeys(this.state, nextState);


why not an @PureRender annotation on the class like we usually do?

That... probably should work? @cmslewis ?

Yep do that!

giladgray · 2017-09-12T18:43:59Z

packages/table/src/cell/formats/truncatedFormat.tsx

@@ -33,6 +34,9 @@ export interface ITruncatedFormatProps extends IProps {
     */
    detectTruncation?: boolean;

+    parentCellHeight?: string;
+    parentCellWidth?: string;


docs? are these props even necessary here? you cast to React.ReactElement<any> above

See comments on PR -- this is explicitly necessary for the shouldComponentUpdate -- we don't actually use them, but we need them to keep from rerendering and remeasuring every time, without knowing if the size has changed.

giladgray · 2017-09-12T18:44:46Z

packages/table/src/cell/cell.tsx

-        const content = <div className={textClasses}>{this.props.children}</div>;
+        const modifiedChildren = React.Children.map(this.props.children, (child) => {
+            if (React.isValidElement(child)) {
+                return React.cloneElement(child as React.ReactElement<any>,


use JSX.Element in place of React.ReactElement<any> if you don't need props type

giladgray · 2017-09-12T18:45:27Z

packages/table/src/cell/cell.tsx

@@ -102,7 +102,15 @@ export class Cell extends React.Component<ICellProps, {}> {
            },
        );

-        const content = <div className={textClasses}>{this.props.children}</div>;
+        const modifiedChildren = React.Children.map(this.props.children, (child) => {
+            if (React.isValidElement(child)) {


hmm i didn't know this method existed. is it necessary? does it actually avoid errors? my suspicion is no, and you don't need it.

It's a typeguard. Without it, child could be react element, or could be text. Hence the return, otherwise.

blueprint-bot · 2017-09-13T16:33:35Z

Change content-width delta 1 26px

Preview: documentation | table
Coverage: core | datetime

gscshoyru · 2017-09-13T17:40:26Z

packages/table/src/cell/formats/truncatedFormat.tsx

+
+        // this is where we get our savings from the injected parent-size props.
+        // if they haven't changed between updates, then no need to remeasure!
+        if (!detectTruncation


This doesn't quite work -- the parent heights and widths may not have changed, but the content could have. You still need to remeasure if anything changed. We can't really get savings this way.

gscshoyru · 2017-09-13T17:50:21Z

packages/table/src/cell/formats/truncatedFormat.tsx

@@ -14,7 +14,7 @@ import * as Classes from "../../common/classes";

 // amount in pixels that the content div width changes when truncated vs when
 // not truncated. Note: could be modified by styles
-const CONTENT_DIV_WIDTH_DELTA = 25;
+const CONTENT_DIV_WIDTH_DELTA = 26;


This is also wrong, it comes from the right property on the div. There's a comment in sass, I'll put one here too.

blueprint-bot · 2017-09-13T18:07:20Z

Undo some of Chris's stuff (sorry Chris!), address comments

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-09-13T20:29:32Z

packages/table/src/cell/formats/truncatedFormat.tsx

@@ -152,21 +165,38 @@ export class TruncatedFormat extends React.Component<ITruncatedFormatProps, ITru
    }

    private setTruncationState() {
-        if (!this.props.detectTruncation || this.props.showPopover !== TruncatedPopoverMode.WHEN_TRUNCATED) {


Whoops, @gscshoyru we'll still want these checks

cmslewis · 2017-09-13T20:30:39Z

packages/table/src/cell/formats/truncatedFormat.tsx

+            || actualContentWidth > containerWidth
+            || actualContentHeight > containerHeight;
+
+        if (isTruncated !== shouldTruncate) {


@gscshoyru I think we can do away with this check now that this is a pure component.

blueprint-bot · 2017-09-13T21:34:15Z

Remove no longer necessary if statement

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-09-13T22:07:44Z

packages/table/src/cell/cell.tsx

+        const modifiedChildren = React.Children.map(this.props.children, (child) => {
+            if (style != null && React.isValidElement(child)) {
+                return React.cloneElement(child as React.ReactElement<any>,
+                    {parentCellHeight: style.height, parentCellWidth: style.width});


nit: proper format would look something like this. but that's why we have prettier incoming.

return React.cloneElement(child as React.ReactElement<any>, { parentCellHeight: style.height, parentCellWidth: style.width, });

blueprint-bot · 2017-09-13T22:11:14Z

Add back check

Preview: documentation | table
Coverage: core | datetime

gscshoyru added 3 commits August 31, 2017 13:51

A slightly different tactic to hopefully fully popover overflow

9cc9134

Refactor as suggested

aa60f4e

Clone cell children and pass them height and width

045746c

gscshoyru assigned cmslewis Sep 11, 2017

cmslewis reviewed Sep 11, 2017

View reviewed changes

Check for null to fix tests

45e1169

giladgray reviewed Sep 12, 2017

View reviewed changes

cmslewis changed the title ~~Gsc/pass height width to formats~~ [Table] Pass parent width/height props to TruncatedFormat Sep 13, 2017

cmslewis added 3 commits September 13, 2017 09:17

Don't measure if the parent size didn't change

a3fb0de

Fix lint, use PureRender

b7a4223

Change content-width delta 1 26px

7c1f60a

gscshoyru commented Sep 13, 2017

View reviewed changes

Undo some of Chris's stuff (sorry Chris!), address comments

235967e

cmslewis reviewed Sep 13, 2017

View reviewed changes

Remove no longer necessary if statement

1d76dcd

Add back check

36c7b5a

cmslewis approved these changes Sep 13, 2017

View reviewed changes

cmslewis mentioned this pull request Sep 13, 2017

[Table] A slightly different tactic to hopefully fully popover overflow #1519

Closed

cmslewis merged commit 6750fcd into master Sep 13, 2017

cmslewis deleted the gsc/pass-height-width-to-formats branch September 13, 2017 22:14

tgreenwatts mentioned this pull request Sep 15, 2017

Table stack overflows at 90% zoom #1459

Closed

llorca mentioned this pull request Sep 15, 2017

Prepare release v1.28.0 #1571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table] Pass parent width/height props to TruncatedFormat #1550

[Table] Pass parent width/height props to TruncatedFormat #1550

gscshoyru commented Sep 11, 2017 •

edited by cmslewis

Loading

cmslewis Sep 11, 2017

cmslewis Sep 11, 2017

cmslewis Sep 11, 2017

cmslewis Sep 13, 2017

blueprint-bot commented Sep 11, 2017

tgreenwatts commented Sep 11, 2017

giladgray Sep 12, 2017

gscshoyru Sep 12, 2017

cmslewis Sep 13, 2017

giladgray Sep 12, 2017

gscshoyru Sep 12, 2017

giladgray Sep 12, 2017

giladgray Sep 12, 2017

gscshoyru Sep 12, 2017

blueprint-bot commented Sep 13, 2017

gscshoyru Sep 13, 2017

gscshoyru Sep 13, 2017

blueprint-bot commented Sep 13, 2017

cmslewis Sep 13, 2017

cmslewis Sep 13, 2017

blueprint-bot commented Sep 13, 2017

cmslewis Sep 13, 2017

blueprint-bot commented Sep 13, 2017

[Table] Pass parent width/height props to TruncatedFormat #1550

[Table] Pass parent width/height props to TruncatedFormat #1550

Conversation

gscshoyru commented Sep 11, 2017 • edited by cmslewis Loading

Changes proposed in this pull request:

Reviewers should focus on:

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

blueprint-bot commented Sep 11, 2017

Check for null to fix tests

tgreenwatts commented Sep 11, 2017

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

blueprint-bot commented Sep 13, 2017

Change content-width delta 1 26px

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 13, 2017

Undo some of Chris's stuff (sorry Chris!), address comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 13, 2017

Remove no longer necessary if statement

Choose a reason for hiding this comment

blueprint-bot commented Sep 13, 2017

Add back check

gscshoyru commented Sep 11, 2017 •

edited by cmslewis

Loading