Skip to content

Use the data-is-scrollable attribute on the correct ScrollablePane div#4602

Merged
ThomasMichon merged 2 commits into
microsoft:masterfrom
elliottsj:scrollable-pane-fix
Apr 23, 2018
Merged

Use the data-is-scrollable attribute on the correct ScrollablePane div#4602
ThomasMichon merged 2 commits into
microsoft:masterfrom
elliottsj:scrollable-pane-fix

Conversation

@elliottsj

@elliottsj elliottsj commented Apr 18, 2018

Copy link
Copy Markdown
Contributor

Pull request checklist

Description of changes

Move the data-is-scrollable attribute to the correct scrollable div: the one which has the overflow-y: auto; CSS property.

Focus areas to test

Run npm start, then visit
http://localhost:4322/#/examples/scrollablepane

The DetailsList example should behave like this:
kapture 2018-04-18 at 13 35 56

@elliottsj

elliottsj commented Apr 18, 2018

Copy link
Copy Markdown
Contributor Author

@MaxLustig can you take a look at this?

@aappddeevv

aappddeevv commented Apr 18, 2018

Copy link
Copy Markdown

I'm looking forward to this merge!

@aappddeevv aappddeevv mentioned this pull request Apr 18, 2018
Closed
ThomasMichon
ThomasMichon requested changes Apr 19, 2018
View reviewed changes

@ThomasMichon ThomasMichon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is going on? Why are 'stickyAbove' and 'stickyBelow' back inside the data-is-scrollable element?

@jordandrako

jordandrako commented Apr 19, 2018

Copy link
Copy Markdown
Contributor

Might fix #4204

@elliottsj

elliottsj commented Apr 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ThomasMichon They are back inside the data-is-scrollable element because the data-is-scrollable attribute needs to be applied to the scrollable div in order for any child List to correctly identify the scrollable div and listen for 'scroll' events.

I'm not clear on why stickyAbove and stickyBelow cannot be inside the data-is-scrollable element. I'm looking at #4055:

This, however, unfurled a different bug in ScrollablePane which the infinite loop had masked: the top sticky header wouldn't properly be placed in stickyAbove on creation of the ScrollablePane, but the constant changing of isStickyTop and isStickyBottom eventually caused it to snap in place. By getting rid of stickyContainer and moving stickyAbove and stickyBelow to siblings of the children of the pane, this is fixed (it also removed the need for resizeContainer).

@MaxLustig Can you help clarify what this means? Is there a repro of this issue I can try?

@leddie24

leddie24 commented Apr 19, 2018

Copy link
Copy Markdown
Collaborator

@elliottsj @ThomasMichon can you look at PR #4491 . This sets data-is-scrollable on the correct element and keeps stickyAbove/Below as sibling children on the scrollable element. #4491 fixes #4601 as well.

@dzearing dzearing closed this Apr 21, 2018
@dzearing dzearing reopened this Apr 21, 2018
@dzearing

dzearing commented Apr 21, 2018

Copy link
Copy Markdown
Member

@leddie24 @ThomasMichon Are there concerns here? Is data-is-scrollable on the wrong element? Seems reasonable to me.

@elliottsj

elliottsj commented Apr 23, 2018

Copy link
Copy Markdown
Contributor Author

@leddie24 Thanks; I'll take a deeper look at your PR when I get a chance. Given that your PR is much bigger (& potentially riskier) than this change, I'd like to get this merged first to at least fix the List scrolling issue.

@ThomasMichon is the stickyAbove / stickyBelow issue blocking here?

@ThomasMichon ThomasMichon merged commit 8d297e2 into microsoft:master Apr 23, 2018
@elliottsj elliottsj deleted the scrollable-pane-fix branch April 23, 2018 21:49
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Apr 26, 2018
@Markionium
Merge branch 'master' into mapol/noGlobalClassNames
4196e29 
* master: (42 commits)
  Applying package updates.
  ProgressIndicator: Finish conversion to mergeStyles (microsoft#4595)
  Fix props validation for Breadcrumb (microsoft#4666)
  No unused vars part of ts (microsoft#4670)
  Picker/Autofill: fixes several minor bugs. (microsoft#4569)
  Fix Calendar component PREV/NEXT month, year, and "Go to today" handlers firing twice (microsoft#4662)
  Applying package updates.
  Merge styles order (microsoft#4664)
  Fabric component: revert class change and make it backwards compatible (microsoft#4671)
  Addressing Issue microsoft#3707 - OverflowSet: Add the ability to set aria-label (microsoft#4667)
  Fix input type for Tile ARIA label prop (microsoft#4668)
  Fix theme slots for DetailsList header colors (microsoft#4658)
  Applying package updates.
  Jolore/calendar updates (microsoft#4643)
  Remove wordWrap setting. (microsoft#4657)
  Pivot: convert to mergeStyles - part 1  (microsoft#4656)
  Use the `data-is-scrollable` attribute on the correct ScrollablePane div (microsoft#4602)
  Applying package updates.
  Remove unused iconClassName prop from Nav.types (microsoft#4634)
  Jest snapshots: classes in animations should autoexpand. (microsoft#4647)
  ...
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Apr 26, 2018
@Markionium
Merge branch 'master' into mapol/allow-overriding-root-element-on-link
6f05b43 
* master: (34 commits)
  Applying package updates.
  ProgressIndicator: Finish conversion to mergeStyles (microsoft#4595)
  Fix props validation for Breadcrumb (microsoft#4666)
  No unused vars part of ts (microsoft#4670)
  Picker/Autofill: fixes several minor bugs. (microsoft#4569)
  Fix Calendar component PREV/NEXT month, year, and "Go to today" handlers firing twice (microsoft#4662)
  Applying package updates.
  Merge styles order (microsoft#4664)
  Fabric component: revert class change and make it backwards compatible (microsoft#4671)
  Addressing Issue microsoft#3707 - OverflowSet: Add the ability to set aria-label (microsoft#4667)
  Fix input type for Tile ARIA label prop (microsoft#4668)
  Fix theme slots for DetailsList header colors (microsoft#4658)
  Applying package updates.
  Jolore/calendar updates (microsoft#4643)
  Remove wordWrap setting. (microsoft#4657)
  Pivot: convert to mergeStyles - part 1  (microsoft#4656)
  Use the `data-is-scrollable` attribute on the correct ScrollablePane div (microsoft#4602)
  Applying package updates.
  Remove unused iconClassName prop from Nav.types (microsoft#4634)
  Jest snapshots: classes in animations should autoexpand. (microsoft#4647)
  ...
@jordandrako jordandrako mentioned this pull request May 3, 2018
Closed
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ThomasMichon ThomasMichon ThomasMichon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Lists rendered inside a ScrollablePane fail to render pages as they are scrolled into view

6 participants

@elliottsj @aappddeevv @jordandrako @leddie24 @dzearing @ThomasMichon