Implement PageLayout component #1820
Conversation
🦋 Changeset detectedLatest commit: e793f10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
.storybook/main.js
Outdated
| 'storybook-addon-performance/register', | ||
| '@whitespace/storybook-addon-html' | ||
| 'storybook-addon-performance/register' | ||
| // '@whitespace/storybook-addon-html' |
There was a problem hiding this comment.
I'll revert this before merging 😅
| name="divider" | ||
| type={`| 'none' | ||
| | 'line'`} | ||
| defaultValue="'none'" |
There was a problem hiding this comment.
if line is the only real alternative, should this be a boolean instead like showDivider?
There was a problem hiding this comment.
Just noticed horizontalDividerVariants. Is this missing 'filled'?
There was a problem hiding this comment.
Yeah, filled is only an option at narrow viewports. I chose divider: none | line instead of showDivider: boolean to align with the dividerWhenNarrow prop.
| import {Box} from '..' | ||
| import {BetterSystemStyleObject, merge, SxProp} from '../sx' | ||
|
|
||
| const REGION_ORDER = { |
There was a problem hiding this comment.
Nitpicking: The uppercasing here feels a bit heavy given it's already private and immutable. Should this be an enum instead?
enum RegionOrder {
Header,
PageStart,
Content
PaneEnd
...
}
| medium: '768px', | ||
| large: '1012px', | ||
| xlarge: '1280px' | ||
| } |
There was a problem hiding this comment.
Just curious, should these come from primitives eventually? I know theme has some breakpoints in there, so how would this correlate back to those in future?
| rowGap = 'normal', | ||
| columnGap = 'normal', | ||
| children, | ||
| sx = {} |
There was a problem hiding this comment.
Should sx have a default value?
There was a problem hiding this comment.
Needed this to make TypeScript happy with the merge() function
src/PageLayout/PageLayout.tsx
Outdated
| }) => { | ||
| return ( | ||
| <PageLayoutContext.Provider value={{outerSpacing, rowGap, columnGap}}> | ||
| <Box sx={merge<BetterSystemStyleObject>({padding: SPACING_MAP[outerSpacing]}, sx)}> |
There was a problem hiding this comment.
Could this lead to a performance issue if we're running this on each render? It feels like it should be hoisted above the return and memoized, given it's a component that will definitely have tons of children. cc. @pksjce what do you think too? Probably overthinking it 😅
There was a problem hiding this comment.
I was also curious about this! Would love to hear what @pksjce thinks
to match with new conventions discussed in primer/react#1820
* Rename inner and outerSpacing props to padding to match with new conventions discussed in primer/react#1820 * Fix Layout example stories * Add header and footer slots to SplitPageLayout Also add initial skeleton for Pull request detail story * add TreeView to Pull request detail story * Add padding: none prop * Ongoing Pull request detail story * Create fifty-chefs-help.md * Resolve storybook conflicts * Resolve storybook conflicts, second try Co-authored-by: Jon Rohan <yes@jonrohan.codes> Co-authored-by: Katie Langerman <langermank@github.com>
This PR implements the PageLayout API discussed in my previous PR.
Note to reviewers: The bulk of the implementation lives in
src/PageLayout/PageLayout.tsx.TODO
sxpropdraftsOut of scope for this PR
refprops