From 1266b108bfa54d219580f18651ed02cd38fc0d87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arma=C4=9Fan?= Date: Wed, 28 Dec 2022 13:01:29 +1000 Subject: [PATCH] PageHeader: Handle responsive CSS properties with breakpoints (#2662) * try using media query instead of hook * Update to use css media queries and make the func more generic * add comments and rename the function * update unit tests * add changeset * remove unused imports * address code review feedback * type fixes & small adj & add fallback CSS declaration * type fix and add comment * update tests * update changeset description * rename the function and move it into utils/ --- .changeset/polite-bats-confess.md | 5 + src/PageHeader/PageHeader.test.tsx | 135 +++++++++------ src/PageHeader/PageHeader.tsx | 105 +++++++++--- .../__snapshots__/PageHeader.test.tsx.snap | 34 +++- src/utils/getBreakpointDeclarations.ts | 161 ++++++++++++++++++ 5 files changed, 355 insertions(+), 85 deletions(-) create mode 100644 .changeset/polite-bats-confess.md create mode 100644 src/utils/getBreakpointDeclarations.ts diff --git a/.changeset/polite-bats-confess.md b/.changeset/polite-bats-confess.md new file mode 100644 index 00000000000..849260b1faf --- /dev/null +++ b/.changeset/polite-bats-confess.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +PageHeader: Add a util function that returns breakpoint styles with the given CSS property and values diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx index 696ea1416fb..4e0cf753526 100644 --- a/src/PageHeader/PageHeader.test.tsx +++ b/src/PageHeader/PageHeader.test.tsx @@ -3,9 +3,7 @@ import '@testing-library/jest-dom/extend-expect' import {render} from '@testing-library/react' import {PageHeader} from '.' import MatchMediaMock from 'jest-matchmedia-mock' -import {behavesAsComponent, checkExports, checkStoriesForAxeViolations} from '../utils/testing' -import {act} from 'react-test-renderer' -import {viewportRanges} from '../hooks/useResponsiveValue' +import {behavesAsComponent, checkExports, checkStoriesForAxeViolations, renderStyles} from '../utils/testing' import {IconButton} from '../Button' import {ChevronLeftIcon, GitBranchIcon, PencilIcon, SidebarExpandIcon} from '@primer/octicons-react' @@ -44,28 +42,63 @@ describe('PageHeader', () => { ) expect(container).toMatchSnapshot() }) - it('does not render ContextArea in wide viewport as default', () => { - act(() => { - matchmedia.useMediaQuery(viewportRanges.wide) - }) + /** These 3 tests below are not following the user behavioural pattern testing paradigm. + * They are testing the internal implementation of the component and checking if the component + * is rendering the correct styles.This approach was necessary due to the impracticality of CSS media queries testing with Jest. + */ + it('respects default visibility of ContextArea and renders CSS media styles correctly', () => { + const expectedStyles = { + '-ms-flex-align': 'center', + '-ms-flex-direction': 'row', + '-ms-flex-order': '0', + '-webkit-align-items': 'center', + '-webkit-box-align': 'center', + '-webkit-flex-direction': 'row', + '-webkit-order': '0', + [`@media screen and (max-width:calc(768px - 0.02px))`]: { + display: 'flex', + }, + [`@media screen and (min-width:768px)`]: { + display: 'none', + }, + 'align-items': 'center', + display: 'flex', + 'flex-direction': 'row', + gap: '0.5rem', + order: '0', + } - const {getByText} = render( - - ContextArea - TitleArea - Description - Navigation - , + expect(renderStyles(ContextArea)).toEqual( + expect.objectContaining(expectedStyles), ) - expect(getByText('ContextArea')).not.toBeVisible() }) - it('respects the hidden prop of ContextArea and renders accordingly', () => { - act(() => { - matchmedia.useMediaQuery(viewportRanges.regular) - }) + it('respects the hidden prop of ContextArea and renders CSS media styles correctly', () => { + const expectedStyles = { + '-ms-flex-align': 'center', + '-ms-flex-direction': 'row', + '-ms-flex-order': '0', + '-webkit-align-items': 'center', + '-webkit-box-align': 'center', + '-webkit-flex-direction': 'row', + '-webkit-order': '0', + [`@media screen and (max-width:calc(768px - 0.02px))`]: { + display: 'flex', + }, + [`@media screen and (min-width:768px)`]: { + display: 'flex', + }, + [`@media screen and (min-width:1400px)`]: { + display: 'none', + }, + 'align-items': 'center', + display: 'flex', + 'flex-direction': 'row', + gap: '0.5rem', + order: '0', + } - const {getByText} = render( - + expect( + renderStyles( - TitleArea - Description - Navigation - , - ) - expect(getByText('ContextArea')).toBeVisible() + , + ), + ).toEqual(expect.objectContaining(expectedStyles)) }) - it('respects default visibility of LeadingAction and TrailingAction and renders accordingly', () => { - act(() => { - matchmedia.useMediaQuery(viewportRanges.narrow) - }) - const {getByTestId} = render( - - ContextArea - - - - - Title - - - - - - - , - ) - expect(getByTestId('LeadingAction')).not.toBeVisible() - expect(getByTestId('TrailingAction')).not.toBeVisible() + it('respects default visibility of LeadingAction and TrailingAction and renders CSS media styles correctly', () => { + const expectedStyles = { + '-ms-flex-align': 'center', + '-webkit-align-items': 'center', + '-webkit-box-align': 'center', + [`@media screen and (max-width:calc(768px - 0.02px))`]: { + display: 'none', + }, + [`@media screen and (min-width:768px)`]: { + display: 'flex', + }, + 'align-items': 'center', + height: '2rem', + } + + expect( + renderStyles( + + + , + ), + ).toEqual(expect.objectContaining(expectedStyles)) }) it('respects the title variant prop', () => { - act(() => { - matchmedia.useMediaQuery(viewportRanges.narrow) - }) const {getByText} = render( ContextArea @@ -120,9 +146,6 @@ describe('PageHeader', () => { expect(getByText('Title')).toHaveStyle('font-size: 2rem') }) it("respects the title variant prop and updates the children components' container height accordingly", () => { - act(() => { - matchmedia.useMediaQuery(viewportRanges.narrow) - }) const {getByText} = render( ContextArea diff --git a/src/PageHeader/PageHeader.tsx b/src/PageHeader/PageHeader.tsx index b93d2eebb3c..ca7e9e8348c 100644 --- a/src/PageHeader/PageHeader.tsx +++ b/src/PageHeader/PageHeader.tsx @@ -6,6 +6,7 @@ import Heading from '../Heading' import {ArrowLeftIcon} from '@primer/octicons-react' import Link from '../Link' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {getBreakpointDeclarations} from '../utils/getBreakpointDeclarations' const REGION_ORDER = { ContextArea: 0, TitleArea: 1, @@ -57,20 +58,22 @@ const Root: React.FC> = ({children, sx // to manage their custom visibility but consumers should be careful if they choose to hide this on narrow viewports. // PageHeader.ContextArea Sub Components: PageHeader.ParentLink, PageHeader.ContextBar, PageHeader.ContextAreaActions // --------------------------------------------------------------------- - const ContextArea: React.FC> = ({ children, hidden = hiddenOnRegularAndWide, sx = {}, }) => { - const isHidden = useResponsiveValue(hidden, false) const contentNavStyles = { - display: isHidden ? 'none' : 'flex', + display: 'flex', flexDirection: 'row', alignItems: 'center', gap: '0.5rem', order: REGION_ORDER.ContextArea, + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), } + return (contentNavStyles, sx)}>{children} } type LinkProps = Pick< @@ -91,7 +94,6 @@ const ParentLink = React.forwardRef( }, ref, ) => { - const isHidden = useResponsiveValue(hidden, false) return ( <> ( muted sx={merge( { - display: isHidden ? 'none' : 'flex', + display: 'flex', alignItems: 'center', gap: '0.5rem', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), }, sx, )} @@ -126,8 +131,21 @@ const ContextBar: React.FC> = ({ sx = {}, hidden = hiddenOnRegularAndWide, }) => { - const isHidden = useResponsiveValue(hidden, false) - return ({display: isHidden ? 'none' : 'flex'}, sx)}>{children} + return ( + ( + { + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), + }, + sx, + )} + > + {children} + + ) } // ContextAreaActions @@ -137,17 +155,19 @@ const ContextAreaActions: React.FC> = ( sx = {}, hidden = hiddenOnRegularAndWide, }) => { - const isHidden = useResponsiveValue(hidden, false) return ( ( { - display: isHidden ? 'none' : 'flex', + display: 'flex', flexDirection: 'row', alignItems: 'center', gap: '0.5rem', flexGrow: '1', justifyContent: 'right', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), }, sx, )} @@ -182,14 +202,21 @@ const TitleArea: React.FC> = ({ hidden = false, variant = 'medium', }) => { - const isHidden = useResponsiveValue(hidden, false) const currentVariant = useResponsiveValue(variant, 'medium') const height = currentVariant === 'large' ? LARGE_TITLE_HEIGHT : MEDIUM_TITLE_HEIGHT return ( ( - {gap: '0.5rem', display: isHidden ? 'none' : 'flex', flexDirection: 'row', alignItems: 'flex-start'}, + { + display: 'flex', + gap: '0.5rem', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), + flexDirection: 'row', + alignItems: 'flex-start', + }, sx, )} > @@ -204,13 +231,19 @@ const LeadingAction: React.FC> = ({ sx = {}, hidden = hiddenOnNarrow, }) => { - const isHidden = useResponsiveValue(hidden, false) const {titleAreaHeight} = React.useContext(TitleAreaContext) return ( ( - {display: isHidden ? 'none' : 'flex', alignItems: 'center', height: titleAreaHeight}, + { + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), + alignItems: 'center', + height: titleAreaHeight, + }, sx, )} > @@ -220,13 +253,15 @@ const LeadingAction: React.FC> = ({ } const LeadingVisual: React.FC> = ({children, sx = {}, hidden = false}) => { - const isHidden = useResponsiveValue(hidden, false) const {titleAreaHeight} = React.useContext(TitleAreaContext) return ( ( { - display: isHidden ? 'none' : 'flex', + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), alignItems: 'center', height: titleAreaHeight, }, @@ -244,7 +279,6 @@ export type TitleProps = { } & PageHeaderProps const Title: React.FC> = ({children, sx = {}, hidden = false, as = 'h3'}) => { - const isHidden = useResponsiveValue(hidden, false) const {titleVariant} = React.useContext(TitleAreaContext) return ( > = ({children, sx = {} medium: '600', subtitle: '400', }[titleVariant], - display: isHidden ? 'none' : 'flex', + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), }, sx, )} @@ -277,14 +314,16 @@ const Title: React.FC> = ({children, sx = {} ) } const TrailingVisual: React.FC> = ({children, sx = {}, hidden = false}) => { - const isHidden = useResponsiveValue(hidden, false) const {titleAreaHeight} = React.useContext(TitleAreaContext) return ( ( { - display: isHidden ? 'none' : 'flex', + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), alignItems: 'center', height: titleAreaHeight, }, @@ -301,13 +340,19 @@ const TrailingAction: React.FC> = ({ sx = {}, hidden = hiddenOnNarrow, }) => { - const isHidden = useResponsiveValue(hidden, false) const {titleAreaHeight} = React.useContext(TitleAreaContext) return ( ( - {display: isHidden ? 'none' : 'flex', alignItems: 'center', height: titleAreaHeight}, + { + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), + alignItems: 'center', + height: titleAreaHeight, + }, sx, )} > @@ -317,13 +362,15 @@ const TrailingAction: React.FC> = ({ } const Actions: React.FC> = ({children, sx = {}, hidden = false}) => { - const isHidden = useResponsiveValue(hidden, false) const {titleAreaHeight} = React.useContext(TitleAreaContext) return ( ( { - display: isHidden ? 'none' : 'flex', + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), flexDirection: 'row', gap: '0.5rem', flexGrow: '1', @@ -341,12 +388,14 @@ const Actions: React.FC> = ({children, // PageHeader.Description: The description area of the header. Visible on all viewports const Description: React.FC> = ({children, sx = {}, hidden = false}) => { - const isHidden = useResponsiveValue(hidden, true) return ( ( { - display: isHidden ? 'none' : 'flex', + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), flexDirection: 'row', alignItems: 'center', gap: '0.5rem', @@ -361,12 +410,14 @@ const Description: React.FC> = ({childr // PageHeader.Navigation: The local navigation area of the header. Visible on all viewports const Navigation: React.FC> = ({children, sx = {}, hidden = false}) => { - const isHidden = useResponsiveValue(hidden, false) return ( ( { - display: isHidden ? 'none' : 'block', + display: 'flex', + ...getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'block' + }), }, sx, )} diff --git a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap index c21d48ac2da..25f75b6d9b1 100644 --- a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap +++ b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap @@ -31,11 +31,11 @@ exports[`PageHeader renders consistently 1`] = ` } .c2 { - gap: 0.5rem; display: -webkit-box; display: -webkit-flex; display: -ms-flexbox; display: flex; + gap: 0.5rem; -webkit-flex-direction: row; -ms-flex-direction: row; flex-direction: row; @@ -64,6 +64,21 @@ exports[`PageHeader renders consistently 1`] = ` display: block; } +@media screen and (max-width:calc(768px - 0.02px)) { + .c1 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + } +} + +@media screen and (min-width:768px) { + .c1 { + display: none; + } +} +
@@ -113,11 +128,11 @@ exports[`PageHeader renders default layout 1`] = ` } .c2 { - gap: 0.5rem; display: -webkit-box; display: -webkit-flex; display: -ms-flexbox; display: flex; + gap: 0.5rem; -webkit-flex-direction: row; -ms-flex-direction: row; flex-direction: row; @@ -146,6 +161,21 @@ exports[`PageHeader renders default layout 1`] = ` display: block; } +@media screen and (max-width:calc(768px - 0.02px)) { + .c1 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + } +} + +@media screen and (min-width:768px) { + .c1 { + display: none; + } +} +
): boolean { + if ('narrow' in responsiveValue && 'regular' in responsiveValue && 'wide' in responsiveValue) { + const values = Object.values(responsiveValue) + return values.every(value => value === values[0]) + } + return false +} +function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue): boolean { + if ('regular' in responsiveValue && 'wide' in responsiveValue) { + return responsiveValue.regular === responsiveValue.wide + } + return false +} + +/** + * This function is inspired by the `useResponsiveValue` hook and it's used to render responsive values with CSS. + * @param value - The value that needs to be rendered responsively + * @param cssProperty - The CSS property whoes value needs to be rendered responsively + * @param mapFn - A function that maps the given value to a CSS value + * + * If the value is responsive, it will only return the given viewports' breakpoints as CSS rules + * with the given CSS property and their mapped value. For viewports that are not specified, + * we need to provide a fallback CSS declaration in the component's sx prop along with the styles + * that will return from this function. + * + * @example + * getBreakpointDeclarations({narrow: true, regular: true, wide: false}, 'display', value => { + return value ? 'none' : 'flex' + }) + * @returns + * { + * "@media screen and (max-width: 768px)": { + * "display": "none" + * }, + * "@media screen and (min-width: 768px)": { + * "display": "none" + * }, + * "@media screen and (min-width: 1440px)": { + * "display": "flex" + * } + * } + * + * * @example + * getBreakpointDeclarations({regular: 'border.default', wide: 'canvas.inset'}, 'backgroundColor', (value): string => { + return value + }) + * @returns + * { + * "@media screen and (min-width: 768px)": { + * "backgroundColor": "border.default" + * }, + * "@media screen and (min-width: 1440px)": { + * "backgroundColor": "canvas.inset" + * } + * } + * + * * @example +* getBreakpointDeclarations({narrow: 'filled', regular: 'line'}, 'height', (value): string => { + return { + filled: 8, + line: 1, + }[value] + }) + * @returns + * { + * "@media screen and (max-width: 768px)": { + * "height": 8 + * } + * "@media screen and (min-width: 768px)": { + * "height": 1 + * }, + * } + * + * If multiple CSS properties need to be rendered responsively in the same CSS rule, this function + * can be called multiple times but make sure to deep merge the CSS declaration objects returned from this function. + * + * * @example + * const mediaQueryStyles = merge( + getBreakpointDeclarations(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), + getBreakpointDeclarations( + { + narrow: 'none', + regular: 'line', + wide: 'filled', + }, + 'backgroundColor', + (value): string => { + return { + none: 'pink', + line: 'salmon', + filled: 'blue', + }[value] + }, + ), + ) + */ +export function getBreakpointDeclarations( + value: TInput | ResponsiveValue, + cssProperty: keyof CSSProperties, + mapFn: (value: TInput) => TOutput, +): BetterSystemStyleObject { + if (isResponsiveValue(value)) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const responsiveValue = value as Extract> + + // Build media queries with the giving cssProperty and mapFn + const narrowMediaQuery = + 'narrow' in responsiveValue + ? { + [`@media screen and ${viewportRanges.narrow}`]: { + [cssProperty]: mapFn(responsiveValue.narrow), + }, + } + : {} + + const regularMediaQuery = + 'regular' in responsiveValue + ? { + [`@media screen and ${viewportRanges.regular}`]: { + [cssProperty]: mapFn(responsiveValue.regular), + }, + } + : {} + + const wideMediaQuery = + 'wide' in responsiveValue + ? { + [`@media screen and ${viewportRanges.wide}`]: { + [cssProperty]: mapFn(responsiveValue.wide), + }, + } + : {} + + // check if all values are the same - this is not a recommended practise but we still should check for it + if (areAllValuesTheSame(responsiveValue)) { + // if all the values are the same, we can just use one of the value to determine the CSS property's value + return {[cssProperty]: mapFn(responsiveValue.narrow)} + // check if regular and wide have the same value, if so we can just return the narrow and regular media queries + } else if (haveRegularAndWideSameValue(responsiveValue)) { + return { + ...narrowMediaQuery, + ...regularMediaQuery, + } + } else { + return { + ...narrowMediaQuery, + ...regularMediaQuery, + ...wideMediaQuery, + } + } + } else { + // If the given value is not a responsive value + return {[cssProperty]: mapFn(value)} + } +}