[Table] Add onCompleteRender prop #1505
Conversation
cmslewis
requested review from
llorca,
giladgray,
themadcreator,
gscshoyru,
avisbal and
tgreenwatts
cmslewis
changed the base branch from
master
to
cl/table-render-optimization-mode
| @@ -452,8 +457,8 @@ export class Table extends AbstractComponent<ITableProps, ITableState> { | |||
|
|
|||
| return !Utils.shallowCompareKeys(this.props, nextProps, propKeysBlacklist) | |||
| || !Utils.shallowCompareKeys(this.state, nextState, stateKeysBlacklist) | |||
| || !Utils.deepCompareKeys(this.props, nextProps, ["selectedRegions"]) | |||
| || !Utils.deepCompareKeys(this.state, nextState, ["selectedRegions"]); | |||
| || !Utils.deepCompareKeys(this.props, nextProps, Table.SHALLOW_COMPARE_PROP_KEYS_BLACKLIST) | |||
There was a problem hiding this comment.
These lists happen to be complementary, so I'm using the shallow-compare blacklists for the deep-compare whitelists. Is that too clever? Should I create separate Table.DEEP_COMPARE_{PROP,STATE}_KEYS_WHITELISTs for the sake of robustness? (I could still set those to equal the shallow-compare blacklist arrays, with a comment to clarify why that's okay.)
There was a problem hiding this comment.
I'm not sure what you mean by this, so... um, possibly a comment anyways? What are you doing that's odd, exactly?
There was a problem hiding this comment.
Eh I think I was overthinking it. Fine as is IMO.
| || (viewportRect == null && nextViewportRect != null); | ||
|
|
||
| if (didViewportChange) { | ||
| this.invokeOnVisibleCellsChangeCallback(nextViewportRect); |
There was a problem hiding this comment.
Noticed we were invoking this even when the viewportRect values hadn't actually changed. This and the shouldComponentUpdate change address that double-invocation issue.
Invoke onVisibleCellsChange only when viewportRect actually changesPreview: documentation | table |
cmslewis
changed the base branch from
cl/table-render-optimization-mode
to
master
cmslewis
force-pushed
the
cl/table-on-complete-render
branch
from
889277b to
1cb0d76
Compare
Fix lintPreview: documentation | table |
| @@ -452,8 +457,8 @@ export class Table extends AbstractComponent<ITableProps, ITableState> { | |||
|
|
|||
| return !Utils.shallowCompareKeys(this.props, nextProps, propKeysBlacklist) | |||
| || !Utils.shallowCompareKeys(this.state, nextState, stateKeysBlacklist) | |||
| || !Utils.deepCompareKeys(this.props, nextProps, ["selectedRegions"]) | |||
| || !Utils.deepCompareKeys(this.state, nextState, ["selectedRegions"]); | |||
| || !Utils.deepCompareKeys(this.props, nextProps, Table.SHALLOW_COMPARE_PROP_KEYS_BLACKLIST) | |||
There was a problem hiding this comment.
I'm not sure what you mean by this, so... um, possibly a comment anyways? What are you doing that's odd, exactly?
Merge branch 'master' into cl/table-on-complete-renderPreview: documentation | table |
Fewer rows so the test won't time outPreview: documentation | table |
Fixes #1495
Checklist
Changes proposed in this pull request:
Tablenow offers anonCompleteRenderprop, invoked on mount/update when all cells in view have finished rendering.Tableno longer firesonVisibleCellsChangeredundantly when the viewport hasn't actually changed.Reviewers should focus on:
MAINquadrant's cells have finished loading, given that theMAINquadrant is a superset of all other quadrants (TOP,LEFT,TOP_LEFT).TableBodyhas a separateBatcher, it's possible to have a race condition wherein theMAINquadrant cells finish rendering before the cells in other, smaller quadrants. One solution there would involve using a single batcher instance passed into theTableBodyfrom the parentTableto properly queue cell rendering across all quadrants; we could also make the quadrants smarter by not double-rendering cells located underneath other quadrants. None of that is implemented in this PR, though, so we'll live with the potential for the race condition for now. What do you think?onCompleteRenderon mount and on update. But looking at the console logs in the table example, I'm not convinced that invoking this callback on update is actually helpful (for instance, the callback is invoked multiple times over the course of a single scroll interaction). InvokingonCompleteRenderonly on mount would still solve resizeByTallest doesn't work on mount because batch loader #1495. What do you think?Screenshot