-
Notifications
You must be signed in to change notification settings - Fork 536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PageLayout: Implement responsive hidden
prop
#2174
Changes from all commits
6383111
eca7133
1ca46d4
30ba162
79c1380
1c683eb
cbcae8f
43fc344
a6b1711
343a5c3
99d8907
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
--- | ||
"@primer/react": minor | ||
--- | ||
|
||
Add a responsive `hidden` prop to `PageLayout.Header`, `PageLayout.Pane`, `PageLayout.Content`, and `PageLayout.Footer` that allows you to hide layout regions based on the viewport width. Example usage: | ||
|
||
```jsx | ||
// Hide pane on narrow viewports | ||
<PageLayout.Pane hidden={{narrow: true}}>...</PageLayout.Pane> | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import React from 'react' | ||
import {BetterSystemStyleObject, merge, SxProp} from '../sx' | ||
import {Box} from '..' | ||
import {ResponsiveValue, useResponsiveValue} from '../hooks/useResponsiveValue' | ||
import {BetterSystemStyleObject, merge, SxProp} from '../sx' | ||
|
||
const REGION_ORDER = { | ||
header: 0, | ||
|
@@ -178,18 +179,22 @@ const VerticalDivider: React.FC<DividerProps> = ({variant = 'none', variantWhenN | |
export type PageLayoutHeaderProps = { | ||
divider?: 'none' | 'line' | ||
dividerWhenNarrow?: 'inherit' | 'none' | 'line' | 'filled' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking nitpick because it's unrelated to this PR: I find it a little strange that this type is different from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll defer to @vdepizzol |
||
hidden?: boolean | ResponsiveValue<boolean> | ||
} & SxProp | ||
|
||
const Header: React.FC<PageLayoutHeaderProps> = ({ | ||
divider = 'none', | ||
dividerWhenNarrow = 'inherit', | ||
hidden = false, | ||
children, | ||
sx = {} | ||
}) => { | ||
const isHidden = useResponsiveValue(hidden, false) | ||
const {rowGap} = React.useContext(PageLayoutContext) | ||
return ( | ||
<Box | ||
as="header" | ||
hidden={isHidden} | ||
sx={merge<BetterSystemStyleObject>( | ||
{ | ||
order: REGION_ORDER.header, | ||
|
@@ -216,6 +221,7 @@ Header.displayName = 'PageLayout.Header' | |
|
||
export type PageLayoutContentProps = { | ||
width?: keyof typeof contentWidths | ||
hidden?: boolean | ResponsiveValue<boolean> | ||
} & SxProp | ||
|
||
// TODO: Account for pane width when centering content | ||
|
@@ -226,10 +232,12 @@ const contentWidths = { | |
xlarge: '1280px' | ||
} | ||
|
||
const Content: React.FC<PageLayoutContentProps> = ({width = 'full', children, sx = {}}) => { | ||
const Content: React.FC<PageLayoutContentProps> = ({width = 'full', hidden = false, children, sx = {}}) => { | ||
const isHidden = useResponsiveValue(hidden, false) | ||
return ( | ||
<Box | ||
as="main" | ||
hidden={isHidden} | ||
sx={merge<BetterSystemStyleObject>( | ||
{ | ||
order: REGION_ORDER.content, | ||
|
@@ -260,6 +268,7 @@ export type PageLayoutPaneProps = { | |
width?: keyof typeof paneWidths | ||
divider?: 'none' | 'line' | ||
dividerWhenNarrow?: 'inherit' | 'none' | 'line' | 'filled' | ||
hidden?: boolean | ResponsiveValue<boolean> | ||
} & SxProp | ||
|
||
const panePositions = { | ||
|
@@ -279,9 +288,11 @@ const Pane: React.FC<PageLayoutPaneProps> = ({ | |
width = 'medium', | ||
divider = 'none', | ||
dividerWhenNarrow = 'inherit', | ||
hidden = false, | ||
children, | ||
sx = {} | ||
}) => { | ||
const isHidden = useResponsiveValue(hidden, false) | ||
const {rowGap, columnGap} = React.useContext(PageLayoutContext) | ||
const computedPositionWhenNarrow = positionWhenNarrow === 'inherit' ? position : positionWhenNarrow | ||
const computedDividerWhenNarrow = dividerWhenNarrow === 'inherit' ? divider : dividerWhenNarrow | ||
|
@@ -293,7 +304,7 @@ const Pane: React.FC<PageLayoutPaneProps> = ({ | |
merge<BetterSystemStyleObject>( | ||
{ | ||
order: panePositions[computedPositionWhenNarrow], | ||
display: 'flex', | ||
display: isHidden ? 'none' : 'flex', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just confirming that we want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe so 👍 cc @hectahertz @alliethu @ichelsea to confirm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's part of the page layout that shouldn't be able to be accessed by anyone, then we'd want to hide if from screen reader users too. |
||
flexDirection: computedPositionWhenNarrow === 'end' ? 'column' : 'column-reverse', | ||
width: '100%', | ||
marginX: 0, | ||
|
@@ -335,18 +346,22 @@ Pane.displayName = 'PageLayout.Pane' | |
export type PageLayoutFooterProps = { | ||
divider?: 'none' | 'line' | ||
dividerWhenNarrow?: 'inherit' | 'none' | 'line' | 'filled' | ||
hidden?: boolean | ResponsiveValue<boolean> | ||
} & SxProp | ||
|
||
const Footer: React.FC<PageLayoutFooterProps> = ({ | ||
divider = 'none', | ||
dividerWhenNarrow = 'inherit', | ||
hidden = false, | ||
children, | ||
sx = {} | ||
}) => { | ||
const isHidden = useResponsiveValue(hidden, false) | ||
const {rowGap} = React.useContext(PageLayoutContext) | ||
return ( | ||
<Box | ||
as="footer" | ||
hidden={isHidden} | ||
sx={merge<BetterSystemStyleObject>( | ||
{ | ||
order: REGION_ORDER.footer, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
import {act, render} from '@testing-library/react' | ||
import MatchMediaMock from 'jest-matchmedia-mock' | ||
import {ResponsiveValue, useResponsiveValue, viewportRanges} from '../../hooks/useResponsiveValue' | ||
import React from 'react' | ||
|
||
let matchMedia: MatchMediaMock | ||
|
||
beforeAll(() => { | ||
matchMedia = new MatchMediaMock() | ||
}) | ||
|
||
afterEach(() => { | ||
matchMedia.clear() | ||
}) | ||
|
||
it('accepts non-responsive values', () => { | ||
const Component = () => { | ||
const value = useResponsiveValue('test', 'fallback') | ||
return <div>{value}</div> | ||
} | ||
|
||
const {getByText} = render(<Component />) | ||
|
||
expect(getByText('test')).toBeInTheDocument() | ||
}) | ||
|
||
it('returns narrow value when viewport is narrow', () => { | ||
const Component = () => { | ||
const value = useResponsiveValue({narrow: false, regular: true} as ResponsiveValue<boolean>, true) | ||
return <div>{JSON.stringify(value)}</div> | ||
} | ||
|
||
// Set narrow viewport | ||
act(() => { | ||
matchMedia.useMediaQuery(viewportRanges.narrow) | ||
}) | ||
|
||
const {getByText} = render(<Component />) | ||
|
||
expect(getByText('false')).toBeInTheDocument() | ||
}) | ||
|
||
it('returns wide value when viewport is wide', () => { | ||
const Component = () => { | ||
const value = useResponsiveValue( | ||
{narrow: 'narrowValue', regular: 'regularValue', wide: 'wideValue'} as ResponsiveValue<string>, | ||
'fallbackValue' | ||
) | ||
return <div>{value}</div> | ||
} | ||
|
||
// Set wide viewport | ||
act(() => { | ||
matchMedia.useMediaQuery(viewportRanges.wide) | ||
}) | ||
|
||
const {getByText} = render(<Component />) | ||
|
||
expect(getByText('wideValue')).toBeInTheDocument() | ||
}) | ||
|
||
it('returns regular value when viewport is regular', () => { | ||
const Component = () => { | ||
const value = useResponsiveValue( | ||
{narrow: 'narrowValue', regular: 'regularValue', wide: 'wideValue'} as ResponsiveValue<string>, | ||
'fallbackValue' | ||
) | ||
return <div>{value}</div> | ||
} | ||
|
||
// Set regular viewport | ||
act(() => { | ||
matchMedia.useMediaQuery(viewportRanges.regular) | ||
}) | ||
|
||
const {getByText} = render(<Component />) | ||
|
||
expect(getByText('regularValue')).toBeInTheDocument() | ||
}) | ||
|
||
it('returns fallback when no value is defined for current viewport', () => { | ||
const Component = () => { | ||
const value = useResponsiveValue( | ||
// Missing value for `regular` viewports | ||
{narrow: 'narrowValue', wide: 'wideValue'} as ResponsiveValue<string>, | ||
'fallbackValue' | ||
) | ||
return <div>{value}</div> | ||
} | ||
|
||
// Set regular viewport | ||
act(() => { | ||
matchMedia.useMediaQuery(viewportRanges.regular) | ||
}) | ||
|
||
const {getByText} = render(<Component />) | ||
|
||
expect(getByText('fallbackValue')).toBeInTheDocument() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking nitpick because it's unrelated to this PR: : What if instead of having
divider
anddividerWhenNarrow
, we just had a divider type that looked like this:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree! That's what I'm planning to do next: