refactor(PageLayout): update sticky layout
Closes https://github.com/github/primer/issues/1264
This PR presents an alternative to our PageLayout.Pane behavior in order to address some of these issues that have come up when using the existing implementation. Specifically, there are two challenges that have been identified:
-
PageLayout.Panewill jump due toposition: sticky
https://user-images.githubusercontent.com/3901764/189767375-5379df0c-bd9d-4652-9d3c-ab71a962b1e3.mov
This is due to the default implementation of position: sticky in the browser. At a certain point, the browser will scroll past the sticky parent and will scroll the overall viewport.
In the existing implementation, the height will re-calc and cause the sticky parent to come back into the viewport. This is what creates the jump as position: sticky comes back into play
- Elements within
PageLayout.Panewill jump around when scrolling
https://user-images.githubusercontent.com/3901764/189767633-2d20570d-9c21-4176-9b51-b81776d566ab.mov
This is due to the height of PageLayout.Pane changing as a result of scrolling. In order to keep the elements in view within the scrolled container, it will attempt to restore the scroll position after the height changes. This leads to a jumpy effect
Proposal
This PR makes some trade-offs to allow for a sticky PageLayout.Pane without some of the challenges described above. Specifically, when encountering PageLayout.Footer (or another footer) the PageLayout.Pane component will follow position: sticky behavior and will not stick to the top.
https://user-images.githubusercontent.com/3901764/189767933-0b0f0768-e121-4cfe-9b0c-400d48dc3ef6.mov
This approach keeps position: sticky in tack and avoids the jumpy-ness of previous attempts but unfortunately will not keep PageLayout.Pane stuck to the top when the footer comes into view as a result
🦋 Changeset detected
Latest commit: 96313cdbdf49301516188c655796c514722c129e
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| @primer/react | Patch |
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
@colebemis one thing I noticed when testing is a z-index issue with the custom sticky header story. What would be the preferred way to address the z-index issue from your perspective? Wasn't sure if there was a system I should be using or if I should just bump the z-index on the custom sticky header and call it a day.
size-limit report 📦
| Path | Size |
|---|---|
| dist/browser.esm.js | 77.66 KB (+0.02% 🔺) |
| dist/browser.umd.js | 78.34 KB (+0.03% 🔺) |
@colebemis one thing I noticed when testing is a
z-indexissue with the custom sticky header story. What would be the preferred way to address thez-indexissue from your perspective? Wasn't sure if there was a system I should be using or if I should just bump thez-indexon the custom sticky header and call it a day.
We don't have any kind of z-index system. Let's bump the z-index and call it a day 😉
Perfect, thanks @colebemis!
Just wanted to follow up here, reached out over in web systems and paired up with @jonrohan to see what we could do with this pattern to avoid the "jumpiness".
When researching more, it seems like this behavior occurs in the original codepen and other implementations across Primer. Below are two examples from different areas:
- https://drive.google.com/file/d/1M8zDpIiJfEmYiqkhbazO2yn_o68G7WkM/view?usp=sharing
- https://drive.google.com/file/d/1yI8H66_TPxxFU6OikXwFsK3-z3-lItah/view?usp=sharing
I tried to take existing videos and slow them down to show where the jump was coming from, hope it makes it clearer!
It seems like the root cause is the time it takes to update the height of the pane in order for position: sticky to get back in. As the amount of content grows in the Pane, the longer it takes for this update to occur.
As an interim trade-off, this PR uses native position: sticky behavior. This leads to the top of the pane not remaining sticky as the footer comes into view. If we can find a way to do this efficiently as content grows then this should be a quick fix to add back in!
Hope that makes sense, let me know if there are any questions!