Add stickyTop prop to PageLayout.Pane by broccolinisoup · Pull Request #2232 · primer/react

broccolinisoup · 2022-08-16T05:47:47Z

Closesthis issue which lives in the internal repo.

Motivation

This PR adds a prop called stickyTop to the PageLayout.Pane to be able to push the pane down and make space for the top sticky element (which lives outside of the PageLayout component) and sticks the pane to a position where the top element ends.

<PageLayout>
<PageLayout.Pane sticky stickyTop={100}>...</PageLayout.Pane>
<PageLayout.Content>...</PageLayout.Content>
</PageLayout>

Screen record

StickyTop.on.PageLayout.Pane.1.mp4

Merge checklist

Added/updated tests
- I'll work with @pksjce and @colebemis to write interaction tests for this case
Added/updated documentation
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge

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-16T05:47:49Z

🦋 Changeset detected

Latest commit: 0adf6b0

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-16T22:59:57Z

size-limit report 📦

Path	Size
dist/browser.esm.js	74.26 KB (+0.08% 🔺)
dist/browser.umd.js	74.82 KB (+0.1% 🔺)

colebemis

Amazing work, @broccolinisoup! 💖 Thank you for taking this on.

Left a couple of minor comments but I don't see any blockers. Feel free to merge this when you feel ready 🚢

broccolinisoup · 2022-08-17T22:47:11Z

+ name="stickyTop"
+ type="number | string"
+ defaultValue="0"
+ description="Use stickyTop to push the sticky pane down to make room for a sticky header if necessary"


Should we add into the description that if it is number, we expect px and if it is string value with units?

Yeah, good idea!

Done! 800b3ec let me know if you have a better wording :)

broccolinisoup · 2022-08-17T22:47:34Z

- calculatedHeight = `calc(100vh - ${Math.max(topOffset, stickyTopHeight) + bottomOffset - overflowScroll}px)`
+ const stickyTopWithUnits = typeof stickyTop === 'number' ? `${stickyTop}px` : stickyTop
+
+ calculatedHeight = `calc(100vh - (max(${topOffset}px, ${stickyTopWithUnits}) + ${bottomOffset}px - ${overflowScroll}px))`


Love that! 🙌🏼

broccolinisoup · 2022-08-17T22:49:22Z

+ }}
 >
-<Heading>Sticky top content</Heading>
+Custom sticky header


Ah this is very explanatory. Thanks for updating it!

Co-authored-by: Cole Bemis <colebemis@github.com>

colebemis · 2022-08-18T00:31:16Z

+ name="stickyTop"
+ type="number | string"
+ defaultValue="0"
+ description="Use stickyTop to push the sticky pane down to make room for a sticky header if necessary. Use the type `string` to specify the height with a unit i.e. 5rem; otherwise the type `number` will be taken as px."


Could you copy this prop documentation and With custom sticky header example into the SplitPageLayout.mdx page as well?

siddharthkp · 2022-08-22T12:17:51Z

Hi!

Sorry, late to the party, how do you feel about calling the prop offset, stickyOffset or even stickyOffsetTop or stickyOffsetHeader instead to make it more clear that we're talking about space.

1.<PageLayout.Paneposition="start"stickyoffset={64}>// because you use it with the sticky prop, ↑ i like this one the most2.<PageLayout.Paneposition="start"stickyoffsetTop={64}>// this is a close second for explicit direction3.<PageLayout.Paneposition="start"stickyoffsetHeader={64}>// i lowkey like this one because it tells you offset from what4.<PageLayout.Paneposition="start"stickystickyOffset={64}>// stick-offset is bit redundant when used with sticky, but more clear5.<PageLayout.Paneposition="start"stickystickyOffsetTop={64}>// this is the most clear we can be 😅 also the most verbose

colebemis · 2022-08-22T21:39:04Z

I like the word "offset" 👍 I think offsetTop is my favorite of the options you listed because it still connects to the top CSS property. I'm curious if it's clear that offsetTop only applies when sticky is true. What do you think @siddharthkp @broccolinisoup?

broccolinisoup · 2022-08-22T21:58:10Z

I like offsetTop and because it needs to be used with the sticky prop I find this line self expressive.

<PageLayout.Pane position="start" sticky offsetTop={64}>
Like "stick my pane to the position start with the top offset 64. "

I am just now thinking we weren't super explicit about stickyTop needs sticky prop to work on the docs. Maybe we should mention that they need to be used together?

@colebemis @siddharthkp

siddharthkp · 2022-08-23T09:56:54Z

Happy with offsetTop

I'm curious if it's clear that offsetTop only applies when sticky is true.

It's not super clear, we can make it more explicit with stickyOffsetTop or we can make the example in our docs better:

Add description under the title "With a custom sticky header", to say what you should change when you have a custom header
without line highlighting, it's not clear which prop in the code helps with custom headers 😓
Flip the order of prop in the example

- <PageLayout.Pane position="start" offsetTop={64} sticky>+ <PageLayout.Pane position="start" sticky offsetTop={64}>

I am just now thinking we weren't super explicit about stickyTop needs sticky prop to work on the docs. Maybe we should mention that they need to be used together?

Yes good point! We should add that to the prop description of offsetTop

broccolinisoup · 2022-08-24T08:22:33Z

Good call to improve the docs!

I created a PR to get the feelings of the new name with the improved docs. Let me know what you think 😊

broccolinisoup added the react label Aug 16, 2022

broccolinisoup requested review from a team and mperrotti August 16, 2022 05:47

broccolinisoup temporarily deployed to github-pages August 16, 2022 05:53 Inactive

Base automatically changed from pagelayout-sticky-prop to mainAugust 16, 2022 06:22

broccolinisoup marked this pull request as draft August 16, 2022 11:36

broccolinisoup added the status: wip label Aug 16, 2022

broccolinisoup temporarily deployed to github-pages August 16, 2022 23:03 Inactive

Add stickyTop prop to PageLayout.Pane
515992a

broccolinisoup force-pushed the pagelayout-stickyTop-prop branch from a59d85a to 515992aCompare August 16, 2022 23:13

broccolinisoup temporarily deployed to github-pages August 16, 2022 23:19 Inactive

update docs
3d1f53b

broccolinisoup temporarily deployed to github-pages August 17, 2022 00:28 Inactive

add changeset
bc54d11

broccolinisoup temporarily deployed to github-pages August 17, 2022 01:23 Inactive

broccolinisoup commented Aug 17, 2022
View reviewed changes

Comment threadsrc/PageLayout/PageLayout.tsx Outdated

broccolinisoup commented Aug 17, 2022
View reviewed changes

Comment threadsrc/PageLayout/useStickyPaneHeight.ts Outdated

broccolinisoup marked this pull request as ready for review August 17, 2022 02:28

broccolinisoup removed the status: wip label Aug 17, 2022

colebemis added 2 commits August 17, 2022 10:46

Update stickyTop story
d06c1a7

Allow stickyTop to accept a string
2d429d1

colebemis approved these changes Aug 17, 2022
View reviewed changes

Comment threaddocs/content/PageLayout.mdx Outdated
Comment threaddocs/content/PageLayout.mdx
Comment threaddocs/content/PageLayout.mdx Outdated

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

broccolinisoup commented Aug 17, 2022
View reviewed changes

Update docs/content/PageLayout.mdx
3919fdd
Co-authored-by: Cole Bemis <colebemis@github.com>

broccolinisoup temporarily deployed to github-pages August 17, 2022 22:59 Inactive

Update docs/content/PageLayout.mdx
e4060b0
Co-authored-by: Cole Bemis <colebemis@github.com>

broccolinisoup temporarily deployed to github-pages August 17, 2022 23:10 Inactive

Merge branch 'main' into pagelayout-stickyTop-prop
c433888

broccolinisoup temporarily deployed to github-pages August 17, 2022 23:23 Inactive

Update prop description on the docs
800b3ec

broccolinisoup temporarily deployed to github-pages August 17, 2022 23:59 Inactive

Merge branch 'main' into pagelayout-stickyTop-prop
d949608

colebemis reviewed Aug 18, 2022
View reviewed changes

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

Add stickyTop example and prop docs
319f756

broccolinisoup temporarily deployed to github-pages August 18, 2022 00:47 Inactive

colebemis reviewed Aug 18, 2022
View reviewed changes

Comment threaddocs/content/SplitPageLayout.mdx Outdated

Update SplitPageLayout example
753a6f1

colebemis reviewed Aug 18, 2022
View reviewed changes

Comment threaddocs/content/SplitPageLayout.mdx

Fix code fence
0adf6b0

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

broccolinisoup merged commit a0fcce6 into mainAug 18, 2022

broccolinisoup deleted the pagelayout-stickyTop-prop branch August 18, 2022 01:08

primer-css mentioned this pull request Aug 18, 2022
Release tracking #2234
Merged

Uh oh!

Conversation

broccolinisoup commented Aug 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Screen record

Merge checklist

Uh oh!

changeset-botBot commented Aug 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actionsBot commented Aug 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Uh oh!

Uh oh!

colebemis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

broccolinisoupAug 17, 2022

Choose a reason for hiding this comment

Uh oh!

colebemisAug 17, 2022

Choose a reason for hiding this comment

Uh oh!

broccolinisoupAug 17, 2022

Choose a reason for hiding this comment

Uh oh!

broccolinisoupAug 17, 2022

Choose a reason for hiding this comment

Uh oh!

broccolinisoupAug 17, 2022

Choose a reason for hiding this comment

Uh oh!

colebemisAug 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

siddharthkp commented Aug 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colebemis commented Aug 22, 2022

Uh oh!

broccolinisoup commented Aug 22, 2022

Uh oh!

siddharthkp commented Aug 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

broccolinisoup commented Aug 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

broccolinisoup commented Aug 16, 2022•
edited
Loading

changeset-botBot commented Aug 16, 2022•
edited
Loading

github-actionsBot commented Aug 16, 2022•
edited
Loading

colebemis left a comment •
edited
Loading

siddharthkp commented Aug 22, 2022•
edited
Loading

siddharthkp commented Aug 23, 2022•
edited
Loading