-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Table] Eliminate unnecessary re-renders everywhere #998
Changes from 14 commits
771bdd1
287c001
07dd7a6
7a46219
0d13d00
244366b
af8d0b2
6dffba3
10b410e
f26744f
8c1ed0e
f3019c3
f749f57
9db6978
3b7c105
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,10 @@ | |
|
||
import { IProps } from "@blueprintjs/core"; | ||
import * as classNames from "classnames"; | ||
import * as PureRender from "pure-render-decorator"; | ||
import * as React from "react"; | ||
import * as Classes from "../common/classes"; | ||
import { IRegion } from "../regions"; | ||
import { Utils } from "../common/utils"; | ||
import { IRegion, Regions } from "../regions"; | ||
|
||
export type IRegionStyler = (region: IRegion) => React.CSSProperties; | ||
|
||
|
@@ -21,13 +21,27 @@ export interface IRegionLayerProps extends IProps { | |
regions?: IRegion[]; | ||
|
||
/** | ||
* A callback interface for applying CSS styles to the regions. | ||
* The array of CSS styles to apply to each region. The ith style object in this array will be | ||
* applied to the ith region in `regions`. | ||
*/ | ||
getRegionStyle: IRegionStyler; | ||
regionStyles?: React.CSSProperties[]; | ||
} | ||
|
||
@PureRender | ||
// don't include "regions" or "regionStyles" in here, because they can't be shallowly compared | ||
const UPDATE_PROPS_KEYS = [ | ||
"className", | ||
]; | ||
|
||
export class RegionLayer extends React.Component<IRegionLayerProps, {}> { | ||
public shouldComponentUpdate(nextProps: IRegionLayerProps) { | ||
// shallowly comparable props like "className" tend not to change in the default table | ||
// implementation, so do that check last with hope that we return earlier and avoid it | ||
// altogether. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
return !Utils.arraysEqual(this.props.regions, nextProps.regions, Regions.regionsEqual) | ||
|| !Utils.arraysEqual(this.props.regionStyles, nextProps.regionStyles, Utils.shallowCompareKeys) | ||
|| !Utils.shallowCompareKeys(this.props, nextProps, UPDATE_PROPS_KEYS); | ||
} | ||
|
||
public render() { | ||
return <div className={Classes.TABLE_OVERLAY_LAYER}>{this.renderRegionChildren()}</div>; | ||
} | ||
|
@@ -40,13 +54,13 @@ export class RegionLayer extends React.Component<IRegionLayerProps, {}> { | |
return regions.map(this.renderRegion); | ||
} | ||
|
||
private renderRegion = (region: IRegion, index: number) => { | ||
const { className, getRegionStyle } = this.props; | ||
private renderRegion = (_region: IRegion, index: number) => { | ||
const { className, regionStyles } = this.props; | ||
return ( | ||
<div | ||
className={classNames(Classes.TABLE_OVERLAY, Classes.TABLE_REGION, className)} | ||
key={index} | ||
style={getRegionStyle(region)} | ||
style={regionStyles[index]} | ||
/> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -528,6 +528,11 @@ export class Regions { | |
return regionGroups; | ||
} | ||
|
||
public static regionsEqual(regionA: IRegion, regionB: IRegion) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why move this? the previous location seems more logical to me: next to all the other comparison checks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's public now, so it has to move up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohhh public gotcha ok |
||
return Regions.intervalsEqual(regionA.rows, regionB.rows) | ||
&& Regions.intervalsEqual(regionA.cols, regionB.cols); | ||
} | ||
|
||
/** | ||
* Iterates over the cells within an `IRegion`, invoking the callback with | ||
* each cell's coordinates. | ||
|
@@ -573,11 +578,6 @@ export class Regions { | |
} | ||
} | ||
|
||
private static regionsEqual(regionA: IRegion, regionB: IRegion) { | ||
return Regions.intervalsEqual(regionA.rows, regionB.rows) | ||
&& Regions.intervalsEqual(regionA.cols, regionB.cols); | ||
} | ||
|
||
private static regionContains(regionA: IRegion, regionB: IRegion) { | ||
// containsRegion expects an array of regions as the first param | ||
return Regions.overlapsRegion([regionA], regionB, false); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any way you can simplify this to all just one function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would literally be the second time I've actually had to use recursion in the wild, ha. Let me dust off my knowledge and give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for this kid. https://www.youtube.com/watch?v=1TZCP6OqRlE
@adidahiya ended up pulling the duplicate validation checks into one place and greatly simplifying the helper function. Didn't do the recursive approach though. This look okay now?