PageLayout.Pane: Add `sticky` prop #2212

colebemis · 2022-08-02T20:48:38Z

Motivation

Pages with a lot of content (examples: files changed tab, settings page, etc.) might want to display a sticky side pane that stays in view as you scroll through the content. This is not currently supported by the PageLayout component

Summary

This PR adds a sticky prop to the PageLayout.Pane component that provides a convenient way for consumers to make side panes sticky:

<PageLayout>
- <PageLayout.Pane>...</PageLayout.Pane>
+ <PageLayout.Pane sticky>...</PageLayout.Pane>
  <PageLayout.Content>...</PageLayout.Content>
</PageLayout>

Here's a quick demo of the sticky behavior:

CleanShot.2022-08-02.at.13.40.33.mp4

Implementation

The sticky is more complex than you might expect so I recorded a quick demo to explain why the complexity exists and how it works:

🎥 🍿 https://share.cleanshot.com/O9uSpc

Merge checklist

~~Added/updated tests~~
- Will add in a separate PR: Add interaction tests for pagelayout sticky #2224 (Thanks @pksjce 🙌 )
Added/updated documentation
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
Test in Projects

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2022-08-02T20:48:40Z

🦋 Changeset detected

Latest commit: 295a1ae

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	Minor

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

github-actions · 2022-08-05T21:41:02Z

size-limit report 📦

Path	Size
dist/browser.esm.js	74.21 KB (+1.7% 🔺)
dist/browser.umd.js	74.75 KB (+1.93% 🔺)

pksjce · 2022-08-11T03:21:57Z

src/PageLayout/useStickyPaneHeight.ts

+
+    // Safari's elastic scroll feature allows you to scroll beyond the scroll height of the page.
+    // We need to account for this when calculating the offset.
+    const overflowScroll = Math.max(window.scrollY + window.innerHeight - document.body.scrollHeight, 0)


Do you think this should be calculated on the scrollcontainer? Or is the PageLayout always meant to be used as a top level component?

PageLayout should always be the top-level component. But this is a good consideration. I'll keep it as window for now and adjust it if needed.

Actually, it looks like memex uses PageLayout inside a scroll container. I'll fix this in this PR

PageLayout should always be the top-level component.

Can you say more about this, would the top level navigation be inside PageLayout.Header?

Right now, I think issues has top level navigation and page layout as siblings, so there's some margin on top

Yeah, I was wrong about PageLayout always being top-level. @joshblack and I just updated the implementation to work if PageLayout is nested in a scroll container: 52e174f

pksjce

I don't have a lot of comments regarding implementation. react-interaction-observer seems like a nifty little library. I wonder why the size-limit did not alert us of the additional 4kb 🤔

Anyways, noticed that this seemed like a great opportunity for some interactive tests!
Here's a PR to this branch with a couple of tests on the storybooks
#2224

For further info check my ADR https://github.com/primer/react/pull/2223/files

siddharthkp

Suggested some improvements for the docs, approving in advance 👍

docs/content/PageLayout.mdx

siddharthkp · 2022-08-12T13:57:32Z

src/PageLayout/PageLayout.test.tsx

@@ -1,6 +1,7 @@
 import React from 'react'
 import {act, render} from '@testing-library/react'
 import MatchMediaMock from 'jest-matchmedia-mock'
+import 'react-intersection-observer/test-utils'


oooh how do we use these?

siddharthkp · 2022-08-12T13:58:12Z

src/PageLayout/PageLayout.tsx

        {children}
      </Box>
+
+      {/* Track the bottom of the content region so we can calculate the height of the pane region */}
+      <Box ref={contentBottomRef} />


thanks for the helpful comment!

siddharthkp · 2022-08-12T14:00:36Z

src/PageLayout/useStickyPaneHeight.ts

+
+    // Safari's elastic scroll feature allows you to scroll beyond the scroll height of the page.
+    // We need to account for this when calculating the offset.
+    const overflowScroll = Math.max(window.scrollY + window.innerHeight - document.body.scrollHeight, 0)


PageLayout should always be the top-level component.

Can you say more about this, would the top level navigation be inside PageLayout.Header?

Right now, I think issues has top level navigation and page layout as siblings, so there's some margin on top

siddharthkp · 2022-08-12T14:03:44Z

src/PageLayout/useStickyPaneHeight.ts

+    const overflowScroll = Math.max(window.scrollY + window.innerHeight - document.body.scrollHeight, 0)
+
+    setHeight(`calc(100vh - ${topOffset + bottomOffset - overflowScroll}px)`)
+  }, [contentTopEntry, contentBottomEntry])


I like this optimisation is a lot!

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

src/PageLayout/PageLayout.stories.tsx

colebemis added 2 commits August 2, 2022 10:55

Add storybook story to test sticky behavior

2bd4a0f

First draft of sticky implementation

68a8934

colebemis had a problem deploying to needs-approval August 2, 2022 20:48 Failure

colebemis temporarily deployed to github-pages August 2, 2022 20:54 Inactive

colebemis added the react label Aug 2, 2022

colebemis added 4 commits August 3, 2022 18:00

Update stories

e1f1db0

Fix overflow

ab32ec4

wip

0bab542

Create useStickyPaneHeight hook

7220176

colebemis had a problem deploying to needs-approval August 5, 2022 21:37 Failure

Update comments

aa74e26

colebemis had a problem deploying to needs-approval August 5, 2022 21:39 Failure

Add TODO comments

62c86b5

colebemis had a problem deploying to needs-approval August 5, 2022 21:42 Failure

colebemis temporarily deployed to github-pages August 5, 2022 21:48 Inactive

colebemis temporarily deployed to github-pages August 5, 2022 21:49 Inactive

Avoid recalculating sticky pane height when sticky is disabled

08f8df7

colebemis had a problem deploying to needs-approval August 8, 2022 21:15 Failure

colebemis temporarily deployed to github-pages August 8, 2022 21:20 Inactive

Calculate intital height

ad0519f

colebemis had a problem deploying to needs-approval August 8, 2022 22:34 Failure

colebemis temporarily deployed to github-pages August 8, 2022 22:40 Inactive

Handle elastic scroll in safari

be186f9

colebemis had a problem deploying to needs-approval August 8, 2022 22:51 Failure

Clean up max() functions

a9fa86d

colebemis had a problem deploying to needs-approval August 8, 2022 22:52 Failure

colebemis temporarily deployed to github-pages August 10, 2022 19:27 Inactive

colebemis had a problem deploying to github-pages August 10, 2022 19:27 Failure

pksjce reviewed Aug 11, 2022

View reviewed changes

Merge branch 'main' into pagelayout-sticky-prop

ebb7ed0

colebemis had a problem deploying to needs-approval August 11, 2022 18:05 Failure

colebemis temporarily deployed to github-pages August 11, 2022 18:11 Inactive

colebemis temporarily deployed to github-pages August 11, 2022 18:12 Inactive

Update snapshots

b07f807

colebemis had a problem deploying to needs-approval August 11, 2022 18:45 Failure

colebemis temporarily deployed to github-pages August 11, 2022 18:51 Inactive

pksjce approved these changes Aug 11, 2022

View reviewed changes

siddharthkp approved these changes Aug 12, 2022

View reviewed changes

Handle nested scroll containers

52e174f

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

colebemis temporarily deployed to github-pages August 16, 2022 00:05 Inactive

colebemis temporarily deployed to github-pages August 16, 2022 00:07 Inactive

colebemis added 3 commits August 15, 2022 17:21

Update sticky prop docs

55fcd74

Update sticky pane docs example

867c9bd

Merge branch 'main' into pagelayout-sticky-prop

41fed71

colebemis temporarily deployed to github-pages August 16, 2022 03:12 Inactive

colebemis commented Aug 16, 2022

View reviewed changes

src/PageLayout/PageLayout.stories.tsx Outdated Show resolved Hide resolved

Update src/PageLayout/PageLayout.stories.tsx

295a1ae

colebemis temporarily deployed to github-pages August 16, 2022 03:31 Inactive

colebemis temporarily deployed to github-pages August 16, 2022 03:32 Inactive

colebemis merged commit 04d9d9c into main Aug 16, 2022

colebemis deleted the pagelayout-sticky-prop branch August 16, 2022 06:22

primer-css mentioned this pull request Aug 16, 2022

Release tracking #2183

Merged

broccolinisoup mentioned this pull request Aug 17, 2022

Add stickyTop prop to PageLayout.Pane #2232

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PageLayout.Pane: Add `sticky` prop #2212

PageLayout.Pane: Add `sticky` prop #2212

colebemis commented Aug 2, 2022 •

edited

Loading

changeset-bot bot commented Aug 2, 2022 •

edited

Loading

github-actions bot commented Aug 5, 2022 •

edited

Loading

pksjce Aug 11, 2022

colebemis Aug 11, 2022

colebemis Aug 11, 2022 •

edited

Loading

siddharthkp Aug 12, 2022

colebemis Aug 16, 2022

pksjce left a comment •

edited

Loading

siddharthkp left a comment

siddharthkp Aug 12, 2022

siddharthkp Aug 12, 2022

siddharthkp Aug 12, 2022

siddharthkp Aug 12, 2022

PageLayout.Pane: Add sticky prop #2212

PageLayout.Pane: Add sticky prop #2212

Conversation

colebemis commented Aug 2, 2022 • edited Loading

Motivation

Summary

Implementation

Merge checklist

changeset-bot bot commented Aug 2, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Aug 5, 2022 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colebemis Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksjce left a comment • edited Loading

Choose a reason for hiding this comment

siddharthkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PageLayout.Pane: Add `sticky` prop #2212

PageLayout.Pane: Add `sticky` prop #2212

colebemis commented Aug 2, 2022 •

edited

Loading

changeset-bot bot commented Aug 2, 2022 •

edited

Loading

github-actions bot commented Aug 5, 2022 •

edited

Loading

colebemis Aug 11, 2022 •

edited

Loading

pksjce left a comment •

edited

Loading