ScrollablePane: Optimization on how component works by leddie24 · Pull Request #4491 · microsoft/fluentui

leddie24 · 2018-04-09T04:37:31Z

Pull request checklist

Addresses an existing issue: Fixes ScrollablePane with DetailsList does not show (virtualized) items #4204
Include a change request file using $ npm run change

Description of changes

Fixes ScrollablePane with DetailsList does not show (virtualized) items #4204
Changes how ScrollablePane calculates whether Sticky components are Sticky or not (does not rely on expensive getBoundingClientRect anymore)
Fixes mounting/unmounting Sticky component after ScrollablePane has already been mounted. No more using unreliable setTimeouts to wait and re-render the pane/Stickies.
Changes scrollablepane positioning to use position: absolute to occupy parent space
Uses MutationObserver to observe DOM changes and update ScrollablePane appropriately.

Focus areas to test

(optional)

…instead of root. Need to fix sticky bottom behavior still.

… setState once. Need to fix DetailsList behavior

… on StickyTop and StickyBottom after mounting/unmounting sticky.

…lsList example. Remove unused componentDidUpdate in Sticky.

…Top or stickyBottom

… stickyTopStyle and stickyBottomStyle

…dden for Top/Bottom sticky

… if props/state changes (preventing excessive setState with updateStickyRefHeights). Change this.root to this.contentContainer for check for initialScrollPosition prop. Move logic for appending child to this.stickyAbove/Below into sortSticky.

…. Fix _removeStickyFromContainers not removing stickies properly.

…hts. Change getScrollPosition to check for contentContainer.scrollTop

…ion and set distances once ScrollablePane has mounted

…c-react into edwl/fixScrollablePaneBehavior # Conflicts: # packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.base.tsx # packages/office-ui-fabric-react/src/components/Sticky/Sticky.tsx

…sing duplicate props.children to placeholder divs

…t changes

… example.

…c-react into edwl/fixScrollablePaneBehavior # Conflicts: # packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.base.tsx # packages/office-ui-fabric-react/src/components/ScrollablePane/examples/ScrollablePane.Default.Example.tsx # packages/office-ui-fabric-react/src/components/Sticky/Sticky.tsx

…c-react into edwl/fixScrollablePaneBehavior # Conflicts: # packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.base.tsx

leddie24 · 2018-05-01T20:58:31Z

@ThomasMichon @MaxLustig can you re-review at your earliest convenience please?

…c-react into edwl/fixScrollablePaneBehavior # Conflicts: # packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.styles.ts

… stickytop/bottom. Add width on sticky component so element doesn't extend past to scrollbar.

ThomasMichon · 2018-05-03T00:49:20Z

packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.styles.ts

    root: [
      classNames.root,
+      {
+        WebkitOverflowScrolling: 'touch',


Should this be a lowercase w?

ThomasMichon · 2018-05-03T00:49:36Z

packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.styles.ts

+    overflowY: 'hidden',
+    overflowX: 'auto'
+  };
+


Can we get rid of the maxHeightStyles altogether now?

…StickyPlaceholder to use position absolute instead of display block/none.

edwlmsft added 18 commits April 5, 2018 14:33

Fix sticky top behavior. Make inner div of ScrollablePane scrollable …

2ae8032

…instead of root. Need to fix sticky bottom behavior still.

Fix sticky footer behavior

2534902

Remove unused state placeholderHeight. clean up onScrollEvent to only…

351b509

… setState once. Need to fix DetailsList behavior

Add stickyPosition prop back

d7befce

Remove stickyBelowContainer from handle.

2e6f1eb

Revert details list example. Fix distanceFromTop calculation

4cc5960

Fix unmounting sticky. Fix ScrollablePane not setting correct heights…

30b8c79

… on StickyTop and StickyBottom after mounting/unmounting sticky.

Update subscribers when DOM changes in scrollablePane. Add back Detai…

de0424b

…lsList example. Remove unused componentDidUpdate in Sticky.

Clean up inline sticky styles

0fe99c9

Remove "testing testing" from default example

92d4db5

Set stickyClassName on nonStickyContent if Sticky component is sticky…

de3070c

…Top or stickyBottom

Remove unused _stickyBelow/_stickyAbove from ScrollablePane. Clean up…

b72e391

… stickyTopStyle and stickyBottomStyle

black to $ms-color-black. Set overflow-x to auto and overflow-y to hi…

e4ab481

…dden for Top/Bottom sticky

Remove unused ref _stickyBelowContainerRef

7fe6b8c

Add getter for contentContainer

7992359

Simplify sortSticky function. Clean up _addtoStickyContainer function…

0082fb4

…. Fix _removeStickyFromContainers not removing stickies properly.

Add back in detailslist example

a143c76

leddie24 requested review from MaxLustig and ThomasMichon April 9, 2018 04:37

edwlmsft added 2 commits April 8, 2018 22:48

Fix linting

ca65251

Remove redundant check for canStickyTop/Bottom in updateStickyRefHeig…

b4a7820

…hts. Change getScrollPosition to check for contentContainer.scrollTop

leddie24 mentioned this pull request Apr 9, 2018

ScrollablePane: Fix StickyAbove/StickyBelow extending past scrollablePane width. Fix DetailsList virtualization rendering #4467

Closed

2 tasks

edwlmsft added 2 commits April 10, 2018 15:48

nit: cleaner code in _addToStickyContainer

697abf8

Fix lint

9e012ff

Jahnp mentioned this pull request Apr 12, 2018

ScrollablePane with DetailsList does not show (virtualized) items #4204

Closed

edwlmsft added 4 commits April 12, 2018 17:10

Add aria-hidden to render for Sticky. Add comment in notifySubscribers.

5d894de

move sortSticky to component mount only. Add setDistanceFromTop funct…

0ff1aec

…ion and set distances once ScrollablePane has mounted

Return undefined instead of null in _getBackground

57faff0

merge master

8741d7c

edwlmsft added 14 commits April 19, 2018 12:52

Remove unused StickyPosition

3799d87

Only updateStickyRefHeights in Sticky if state changes. Change from u…

a7aeb07

…sing duplicate props.children to placeholder divs

Add mutationObserver to check for Sticky component state changes

aac9e53

Change scrollablePane didUpdate to notify when stickyTop/Bottom heigh…

0080b46

…t changes

Update scrollablePane styles to use position absolute. Update default…

0313824

… example.

Fix default example to not have duplicate colors

28709a7

Update details list example

78a298f

Update ScrollablePane Do's. Update example css

59f2ab7

Update sticky donts

0ef10da

Fix lint

b8aab4c

more lint

489d246

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

ac905dc

…c-react into edwl/fixScrollablePaneBehavior # Conflicts: # packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.base.tsx

edwlmsft added 4 commits May 2, 2018 14:12

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

474bf81

…c-react into edwl/fixScrollablePaneBehavior # Conflicts: # packages/office-ui-fabric-react/src/components/ScrollablePane/ScrollablePane.styles.ts

Set refs as private properties of Sticky. Remove background #fff from…

1fa5a01

… stickytop/bottom. Add width on sticky component so element doesn't extend past to scrollbar.

Fix order of public/private in Sticky

e04d2a7

Set width on stickyabove/below instead of Sticky component itself.

00a8b44

ThomasMichon reviewed May 3, 2018

View reviewed changes

ThomasMichon approved these changes May 3, 2018

View reviewed changes

edwlmsft added 3 commits May 3, 2018 16:31

Get rid of maxheight styles. Add throttle to scroll event. Change non…

b49fa90

…StickyPlaceholder to use position absolute instead of display block/none.

Update Do's

c5b0ef3

Add change file

f37feb8

leddie24 merged commit 34683b1 into microsoft:master May 4, 2018

leddie24 deleted the edwl/fixScrollablePaneBehavior branch May 4, 2018 00:25

leddie24 mentioned this pull request May 4, 2018

ScrollablePane, DetailsList and Sticky Header: blank space inserted and sticky obscures 1st item #4234

Closed

leddie24 mentioned this pull request Jun 1, 2018

ScrollablePane with DetailsList and Sticky Header is overpainting scrollbar #4205

Closed

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ScrollablePane: Optimization on how component works#4491

ScrollablePane: Optimization on how component works#4491
leddie24 merged 48 commits intomicrosoft:masterfrom
leddie24:edwl/fixScrollablePaneBehavior

leddie24 commented Apr 9, 2018 •

edited

Loading

Uh oh!

leddie24 commented May 1, 2018

Uh oh!

ThomasMichon May 3, 2018

Uh oh!

ThomasMichon May 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

leddie24 commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

leddie24 commented May 1, 2018

Uh oh!

ThomasMichon May 3, 2018

Choose a reason for hiding this comment

Uh oh!

ThomasMichon May 3, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

leddie24 commented Apr 9, 2018 •

edited

Loading