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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Apr 23, 2018
Merged

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

merged 2 commits into from
Apr 23, 2018

Conversation

elliottsj
Copy link
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
Copy link
Contributor Author

@MaxLustig can you take a look at this?

@aappddeevv
Copy link

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
Copy link
Member

@ThomasMichon ThomasMichon left a comment

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
Copy link
Contributor

Might fix #4204

@elliottsj
Copy link
Contributor Author

elliottsj commented Apr 19, 2018 •
edited
Loading

@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
Copy link
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
Copy link
Member

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

@elliottsj
Copy link
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