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] Add renderMode prop #1503

Merged
merged 7 commits into from
Aug 28, 2017
Merged

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Aug 25, 2017

Addresses #1374, #1415, #1495

Checklist

  • Include tests
  • Update documentation (contained in new prop's description)

Changes proposed in this pull request:

  • 💎 NEW Table now supports a renderMode?: RenderMode prop:
    • RenderMode.BATCH (default): renders cells in batches to improve performance
    • RenderMode.NONE: renders cells synchronously all at once
  • Added a new "Batch rendering" switch to the Table example:
    image

Reviewers should focus on:

I designed the prop as a RenderMode enum instead of a useBatchRendering boolean to allow us to support additional rendering modes in the future. See: #1374 (comment), pasted here for your convenience:

Another idea is, instead of a prop useBatchRendering?: boolean;, to add a renderOptimizationMode?: RenderOptimizationMode; prop (working title) that accepts values like:

  • RenderOptimizationMode.BATCH: render as many new cells as possible per frame, and no more (this is what we currently have, and this scheme also has the strongest guarantee against bringing the UI to a crawl)
  • RenderOptimizationMode.DEBOUNCE: debounce scrolling, hiding cells on scroll start and rendering cells all at once after the scroll event finishes (this has a risk of locking the UI if the cells are small/numerous/complex/have truncated content, e.g.)
  • RenderOptimizationMode.NONE: no optimizations, just render everything (useful for smaller tables, but can easily bring the UI to a crawl on larger tables)

(Maybe also a RenderOptimizationMode.THROTTLE for throttling scrolling).

@blueprint-bot
Copy link

Fix lint

Preview: documentation | table
Coverage: core | datetime

@@ -42,6 +42,7 @@ ReactDOM.render(<Nav selected="perf" />, document.getElementById("nav"));

import { IFocusedCellCoordinates } from "../src/common/cell";
import { IColumnIndices, IRowIndices } from "../src/common/grid";
import { RenderOptimizationMode } from "../src/common/renderOptimizationMode";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just call it render mode? And mention that we do optimizations in the docs? That way we can get rid of the extra mouthful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@blueprint-bot
Copy link

Rename to renderMode

Preview: documentation | table
Coverage: core | datetime

@cmslewis cmslewis changed the title [Table] Add renderOptimizationMode prop [Table] Add renderMode prop Aug 28, 2017
Copy link
Contributor

@tgreenwatts tgreenwatts left a comment

Choose a reason for hiding this comment

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

:shipit:

@cmslewis cmslewis merged commit cf4b2a6 into master Aug 28, 2017
@cmslewis cmslewis mentioned this pull request Aug 30, 2017
@adidahiya adidahiya deleted the cl/table-render-optimization-mode branch September 22, 2017 19:05
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