react icon indicating copy to clipboard operation
react copied to clipboard

refactor(PageLayout): update sticky layout

Open joshblack opened this issue 3 years ago • 5 comments

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:

  1. PageLayout.Pane will jump due to position: 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

  1. Elements within PageLayout.Pane will 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

joshblack avatar Sep 12 '22 22:09 joshblack

🦋 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

changeset-bot[bot] avatar Sep 12 '22 22:09 changeset-bot[bot]

@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.

joshblack avatar Sep 12 '22 22:09 joshblack

size-limit report 📦

Path Size
dist/browser.esm.js 77.66 KB (+0.02% 🔺)
dist/browser.umd.js 78.34 KB (+0.03% 🔺)

github-actions[bot] avatar Sep 12 '22 22:09 github-actions[bot]

@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.

We don't have any kind of z-index system. Let's bump the z-index and call it a day 😉

colebemis avatar Sep 13 '22 16:09 colebemis

Perfect, thanks @colebemis!

joshblack avatar Sep 14 '22 17:09 joshblack

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!

joshblack avatar Oct 12 '22 19:10 joshblack