Skip to content
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] Introduce TableBodyCells component to avoid unnecessary cell renders #1596

Merged
merged 5 commits into from
Sep 26, 2017

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Sep 21, 2017

Addresses #1547

Checklist

  • Include tests (no need; just an under-the-hood refactor)

Changes proposed in this pull request:

Before:

  • TableBody used to invoke props.renderCell for every cell in view, every time selectedRegions changed, just to run a diff against the currently rendered cells. That's a lot of wasted work for an interaction that doesn't affect the cells themselves.

After:

  • The TableBodyCells component now encapsulates all cell rendering and doesn't respond to selectedRegions changes. This means that we now do zero work in the TableBody when that prop changes. Woo!
    • This also greatly simplifies our shouldComponentUpdate logic in TableBody.

Reviewers should focus on:

  • Look at the React Dev Tools in Chrome, and tick the "Highlight updates" checkbox.
  • Click to move the selection around.
  • Note that nothing in the table body is highlighted, because nothing in the table body repaints on the selectedRegions change anymore.

Screenshot

2017-09-21 16 22 02

@cmslewis cmslewis added this to the 1.29.0 milestone Sep 21, 2017
@cmslewis cmslewis changed the title [Table] Introduce TableBodyCells component to avoid unnecessary cell-renders [Table] Introduce TableBodyCells component to avoid unnecessary cell renders Sep 21, 2017
Copy link
Contributor

@gscshoyru gscshoyru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear: mostly, all you did was made a different separation of concerns for rendering cells vs rendering regions, right? So that changing regions doesn't re-render cells? (which is a boat load of work)

Looks good to me!

@cmslewis
Copy link
Contributor Author

FAILED TESTS:
    onCompleteRender
      ✖ triggers immediately on mount/update with RenderMode.BATCH for very small batches

Hmm, will look into this after lunch.

@blueprint-bot
Copy link

Fix tests

Preview: documentation | table
Coverage: core | datetime

Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preview looks good. All those unnecessary renders cut!

@blueprint-bot
Copy link

Merge branch 'master' into cl/table-body-cells-component

Preview: documentation | table
Coverage: core | datetime

@cmslewis cmslewis merged commit 215b991 into master Sep 26, 2017
@cmslewis cmslewis deleted the cl/table-body-cells-component branch September 26, 2017 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants