Skip to content

Conversation

@KevinTCoughlin
Copy link
Member

@KevinTCoughlin KevinTCoughlin commented Mar 22, 2019

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

We can render GroupSpacer less and save row rendering time for DetailsList & GroupedList.

Test Case

Rendering DetailsList.Basic.Example w/ 10,000 rows

Before Changes:

before

After Changes:

The only time the profiler reports time-spent in GroupSpacer is eval'ing the modules:

image

What is a GroupSpacer

This space (made blue for snapshot):

image

Backwards Compatibility

We lose the ability to theme this empty space, but I feel like the performance benefits are worth it.

I explored conditionally rendering the component to preserve backwards-compatibility in 580bb08.

Focus areas to test

  • CI should pass
  • DetailsList non-grouped should render as expected
  • DetailsList grouped should render with indentation as it does on master
  • GroupedList should render with indentation as it does on master
Microsoft Reviewers: Open in CodeFlow

@KevinTCoughlin
Copy link
Member Author

@JasonGore may be interested in this perf as it relates to styled-HOC

@KevinTCoughlin KevinTCoughlin changed the title DetailsList's GroupSpacer: Refactor to React.SFC & Remove styled HOC to improve render performance DetailsList's GroupSpacer: Refactor to React.SFC & remove styled HOC to improve render performance Mar 22, 2019
@size-auditor
Copy link

size-auditor bot commented Mar 22, 2019

Bundle test Size (minified) Diff from master
ShimmeredDetailsList 219.74 kB BelowBaseline     -174 bytes
GroupedList 118.838 kB BelowBaseline     -175 bytes
DetailsList 208.916 kB BelowBaseline     -176 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

@KevinTCoughlin
Copy link
Member Author

KevinTCoughlin commented Mar 22, 2019

Puppeteer would be awesome. This is a great candidate for a test suite, especially with CPU throttling.

@KevinTCoughlin
Copy link
Member Author

KevinTCoughlin commented Mar 22, 2019

Test Case 2

Results from DetailsList.Basic.Example w/ 1,000 items and virtualization disabled

Before

image

After

image

* Run by Kevin-bot 😆

Note: I tried 10,000 items with virtualization disabled, but my browser broke.

Copy link
Contributor

@Vitalius1 Vitalius1 left a comment

Choose a reason for hiding this comment

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

I like the change. The only thing I am worried about is that we lost a simple way to fluentize this part in fluent-theme package through Customizer with the scope name (the width changes in fluent styles), but I think it's worth it as I don't need it for fabric-7 branch.

@KevinTCoughlin
Copy link
Member Author

FWIW this customization hook for GroupSpacer is unused across OneDrive / SharePoint

@KevinTCoughlin
Copy link
Member Author

Test Case 3

Results from GroupedList.Basic.Example w/ 3 groups of depth 3 and virtualization enabled (defaults).

Before

image

After

image

I tried larger variants of the GroupedList, but they were mostly unusable ☹️.

@dzearing dzearing merged commit 03b32b4 into microsoft:master Mar 22, 2019
@KevinTCoughlin KevinTCoughlin deleted the keco/conditionally-render-groupspacer branch March 23, 2019 00:00
@msft-github-bot
Copy link
Contributor

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants