From 9a91eaded4e2678027282f76c9518691d5a46647 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Tue, 6 Dec 2022 14:22:41 +1000 Subject: [PATCH 01/12] try using media query instead of hook --- src/PageHeader/PageHeader.tsx | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/PageHeader/PageHeader.tsx b/src/PageHeader/PageHeader.tsx index b93d2eebb3c..21fdb676e1e 100644 --- a/src/PageHeader/PageHeader.tsx +++ b/src/PageHeader/PageHeader.tsx @@ -1,6 +1,6 @@ import React from 'react' import {Box} from '..' -import {useResponsiveValue, ResponsiveValue} from '../hooks/useResponsiveValue' +import {isResponsiveValue, useResponsiveValue, ResponsiveValue} from '../hooks/useResponsiveValue' import {SxProp, merge, BetterSystemStyleObject} from '../sx' import Heading from '../Heading' import {ArrowLeftIcon} from '@primer/octicons-react' @@ -53,23 +53,43 @@ const Root: React.FC> = ({children, sx ) } +function displayResponsively(value: T, fallback: F): BetterSystemStyleObject { + if (isResponsiveValue(value)) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const responsiveValue = value as Extract> + + return { + [`@media screen and (max-width: 768px)`]: { + display: 'narrow' in responsiveValue ? (responsiveValue.narrow ? 'none' : 'flex') : fallback ? 'none' : 'flex', + }, + [`@media screen and (min-width: 768px)`]: { + display: + 'regular' in responsiveValue ? (responsiveValue.regular ? 'none' : 'flex') : fallback ? 'none' : 'flex', + }, + [`@media screen and (min-width: 1440px)`]: { + display: 'wide' in responsiveValue ? (responsiveValue.wide ? 'none' : 'flex') : fallback ? 'none' : 'flex', + }, + } + } else { + return {display: fallback ? 'none' : 'flex'} + } +} + // PageHeader.ContextArea : Only visible on narrow viewports by default to provide user context of where they are at their journey. `visible` prop available // 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', flexDirection: 'row', alignItems: 'center', gap: '0.5rem', order: REGION_ORDER.ContextArea, + ...displayResponsively(hidden, false), } return (contentNavStyles, sx)}>{children} } From 6986cf2d8fed9e14175403ad220d048c56742617 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 9 Dec 2022 14:32:57 +1000 Subject: [PATCH 02/12] Update to use css media queries and make the func more generic --- src/PageHeader/PageHeader.test.tsx | 20 ++--- src/PageHeader/PageHeader.tsx | 87 +++++++------------ .../__snapshots__/PageHeader.test.tsx.snap | 50 +++++++++-- src/PageHeader/manageResponsiveVisibility.ts | 37 ++++++++ 4 files changed, 119 insertions(+), 75 deletions(-) create mode 100644 src/PageHeader/manageResponsiveVisibility.ts diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx index 696ea1416fb..57a16150342 100644 --- a/src/PageHeader/PageHeader.test.tsx +++ b/src/PageHeader/PageHeader.test.tsx @@ -44,7 +44,7 @@ describe('PageHeader', () => { ) expect(container).toMatchSnapshot() }) - it('does not render ContextArea in wide viewport as default', () => { + it.skip('does not render ContextArea in wide viewport as default', () => { act(() => { matchmedia.useMediaQuery(viewportRanges.wide) }) @@ -57,11 +57,12 @@ describe('PageHeader', () => { Navigation , ) - expect(getByText('ContextArea')).not.toBeVisible() + expect(getByText('ContextArea')).toHaveStyle('display: none') + // expect(getByText('ContextArea')).not.toBeVisible() }) - it('respects the hidden prop of ContextArea and renders accordingly', () => { + it.skip('respects the hidden prop of ContextArea and renders accordingly', () => { act(() => { - matchmedia.useMediaQuery(viewportRanges.regular) + matchmedia.useMediaQuery(viewportRanges.wide) }) const {getByText} = render( @@ -80,9 +81,10 @@ describe('PageHeader', () => { Navigation , ) - expect(getByText('ContextArea')).toBeVisible() + expect(getByText('ContextArea')).not.toBeVisible() + expect(getByText('ContextArea')).toHaveStyle('display: none') }) - it('respects default visibility of LeadingAction and TrailingAction and renders accordingly', () => { + it.skip('respects default visibility of LeadingAction and TrailingAction and renders accordingly', () => { act(() => { matchmedia.useMediaQuery(viewportRanges.narrow) }) @@ -106,9 +108,6 @@ describe('PageHeader', () => { expect(getByTestId('TrailingAction')).not.toBeVisible() }) it('respects the title variant prop', () => { - act(() => { - matchmedia.useMediaQuery(viewportRanges.narrow) - }) const {getByText} = render( ContextArea @@ -120,9 +119,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 21fdb676e1e..6c160e7f8ae 100644 --- a/src/PageHeader/PageHeader.tsx +++ b/src/PageHeader/PageHeader.tsx @@ -1,11 +1,12 @@ import React from 'react' import {Box} from '..' -import {isResponsiveValue, useResponsiveValue, ResponsiveValue} from '../hooks/useResponsiveValue' +import {useResponsiveValue, ResponsiveValue} from '../hooks/useResponsiveValue' import {SxProp, merge, BetterSystemStyleObject} from '../sx' import Heading from '../Heading' import {ArrowLeftIcon} from '@primer/octicons-react' import Link from '../Link' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {displayResponsively} from './manageResponsiveVisibility' const REGION_ORDER = { ContextArea: 0, TitleArea: 1, @@ -53,28 +54,6 @@ const Root: React.FC> = ({children, sx ) } -function displayResponsively(value: T, fallback: F): BetterSystemStyleObject { - if (isResponsiveValue(value)) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const responsiveValue = value as Extract> - - return { - [`@media screen and (max-width: 768px)`]: { - display: 'narrow' in responsiveValue ? (responsiveValue.narrow ? 'none' : 'flex') : fallback ? 'none' : 'flex', - }, - [`@media screen and (min-width: 768px)`]: { - display: - 'regular' in responsiveValue ? (responsiveValue.regular ? 'none' : 'flex') : fallback ? 'none' : 'flex', - }, - [`@media screen and (min-width: 1440px)`]: { - display: 'wide' in responsiveValue ? (responsiveValue.wide ? 'none' : 'flex') : fallback ? 'none' : 'flex', - }, - } - } else { - return {display: fallback ? 'none' : 'flex'} - } -} - // PageHeader.ContextArea : Only visible on narrow viewports by default to provide user context of where they are at their journey. `visible` prop available // 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 @@ -89,7 +68,7 @@ const ContextArea: React.FC> = ({ alignItems: 'center', gap: '0.5rem', order: REGION_ORDER.ContextArea, - ...displayResponsively(hidden, false), + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), } return (contentNavStyles, sx)}>{children} } @@ -111,7 +90,6 @@ const ParentLink = React.forwardRef( }, ref, ) => { - const isHidden = useResponsiveValue(hidden, false) return ( <> ( muted sx={merge( { - display: isHidden ? 'none' : 'flex', alignItems: 'center', gap: '0.5rem', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), }, sx, )} @@ -146,8 +124,11 @@ const ContextBar: React.FC> = ({ sx = {}, hidden = hiddenOnRegularAndWide, }) => { - const isHidden = useResponsiveValue(hidden, false) - return ({display: isHidden ? 'none' : 'flex'}, sx)}>{children} + return ( + (displayResponsively(hidden, false, 'display', 'none', 'flex'), sx)}> + {children} + + ) } // ContextAreaActions @@ -157,17 +138,16 @@ const ContextAreaActions: React.FC> = ( sx = {}, hidden = hiddenOnRegularAndWide, }) => { - const isHidden = useResponsiveValue(hidden, false) return ( ( { - display: isHidden ? 'none' : 'flex', flexDirection: 'row', alignItems: 'center', gap: '0.5rem', flexGrow: '1', justifyContent: 'right', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), }, sx, )} @@ -202,14 +182,18 @@ 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'}, + { + gap: '0.5rem', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + flexDirection: 'row', + alignItems: 'flex-start', + }, sx, )} > @@ -224,13 +208,16 @@ 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}, + { + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + alignItems: 'center', + height: titleAreaHeight, + }, sx, )} > @@ -240,13 +227,12 @@ 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', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), alignItems: 'center', height: titleAreaHeight, }, @@ -264,7 +250,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', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), }, sx, )} @@ -297,14 +282,13 @@ 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', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), alignItems: 'center', height: titleAreaHeight, }, @@ -321,13 +305,16 @@ 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}, + { + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + alignItems: 'center', + height: titleAreaHeight, + }, sx, )} > @@ -337,13 +324,12 @@ 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', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), flexDirection: 'row', gap: '0.5rem', flexGrow: '1', @@ -361,12 +347,11 @@ 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', + ...displayResponsively(hidden, false, 'display', 'none', 'flex'), flexDirection: 'row', alignItems: 'center', gap: '0.5rem', @@ -381,16 +366,8 @@ 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', - }, - sx, - )} - > + (displayResponsively(hidden, false, 'display', 'none', 'block'), sx)}> {children} ) diff --git a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap index c21d48ac2da..3de7753c0a5 100644 --- a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap +++ b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap @@ -13,10 +13,6 @@ exports[`PageHeader renders consistently 1`] = ` } .c1 { - display: -webkit-box; - display: -webkit-flex; - display: -ms-flexbox; - display: flex; -webkit-flex-direction: row; -ms-flex-direction: row; flex-direction: row; @@ -64,6 +60,27 @@ exports[`PageHeader renders consistently 1`] = ` display: block; } +@media screen and (max-width:768px) { + .c1 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + } +} + +@media screen and (min-width:768px) { + .c1 { + display: none; + } +} + +@media screen and (min-width:1440px) { + .c1 { + display: none; + } +} +
@@ -95,10 +112,6 @@ exports[`PageHeader renders default layout 1`] = ` } .c1 { - display: -webkit-box; - display: -webkit-flex; - display: -ms-flexbox; - display: flex; -webkit-flex-direction: row; -ms-flex-direction: row; flex-direction: row; @@ -146,6 +159,27 @@ exports[`PageHeader renders default layout 1`] = ` display: block; } +@media screen and (max-width:768px) { + .c1 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + } +} + +@media screen and (min-width:768px) { + .c1 { + display: none; + } +} + +@media screen and (min-width:1440px) { + .c1 { + display: none; + } +} +
( + // what is your value that you want to render responsivley? - responsiveValue + value: T, + fallback: F, + cssProperty: string, + truety: B, + falsy: C, +): BetterSystemStyleObject { + if (isResponsiveValue(value)) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const responsiveValue = value as Extract> + const checkFallback = fallback ? truety : falsy + + // arguments + // what is the value that we want to render responsively? - responsiveValue + // what is your css property? - display + // truety value - none + // falsy value - flex + + return { + [`@media screen and (max-width: 768px)`]: { + [cssProperty]: 'narrow' in responsiveValue ? (responsiveValue.narrow ? truety : falsy) : checkFallback, + }, + [`@media screen and (min-width: 768px)`]: { + [cssProperty]: 'regular' in responsiveValue ? (responsiveValue.regular ? truety : falsy) : checkFallback, + }, + [`@media screen and (min-width: 1440px)`]: { + [cssProperty]: 'wide' in responsiveValue ? (responsiveValue.wide ? truety : falsy) : checkFallback, + }, + } + } else { + return {[cssProperty]: value ? truety : falsy} + } +} From 26f819a781f0c002206e9757c9d505d4c3ffe870 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Mon, 12 Dec 2022 11:43:00 +1000 Subject: [PATCH 03/12] add comments and rename the function --- src/PageHeader/PageHeader.tsx | 28 ++++++++-------- ...ity.ts => manageResponsiveValueWithCSS.ts} | 32 ++++++++++++++----- 2 files changed, 38 insertions(+), 22 deletions(-) rename src/PageHeader/{manageResponsiveVisibility.ts => manageResponsiveValueWithCSS.ts} (54%) diff --git a/src/PageHeader/PageHeader.tsx b/src/PageHeader/PageHeader.tsx index 6c160e7f8ae..e3e24629d60 100644 --- a/src/PageHeader/PageHeader.tsx +++ b/src/PageHeader/PageHeader.tsx @@ -6,7 +6,7 @@ import Heading from '../Heading' import {ArrowLeftIcon} from '@primer/octicons-react' import Link from '../Link' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' -import {displayResponsively} from './manageResponsiveVisibility' +import {CSSManagedResponsiveValue} from './manageResponsiveValueWithCSS' const REGION_ORDER = { ContextArea: 0, TitleArea: 1, @@ -68,7 +68,7 @@ const ContextArea: React.FC> = ({ alignItems: 'center', gap: '0.5rem', order: REGION_ORDER.ContextArea, - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), } return (contentNavStyles, sx)}>{children} } @@ -101,7 +101,7 @@ const ParentLink = React.forwardRef( { alignItems: 'center', gap: '0.5rem', - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), }, sx, )} @@ -125,7 +125,7 @@ const ContextBar: React.FC> = ({ hidden = hiddenOnRegularAndWide, }) => { return ( - (displayResponsively(hidden, false, 'display', 'none', 'flex'), sx)}> + (CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), sx)}> {children} ) @@ -147,7 +147,7 @@ const ContextAreaActions: React.FC> = ( gap: '0.5rem', flexGrow: '1', justifyContent: 'right', - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), }, sx, )} @@ -190,7 +190,7 @@ const TitleArea: React.FC> = ({ sx={merge( { gap: '0.5rem', - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), flexDirection: 'row', alignItems: 'flex-start', }, @@ -214,7 +214,7 @@ const LeadingAction: React.FC> = ({ ( { - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), alignItems: 'center', height: titleAreaHeight, }, @@ -232,7 +232,7 @@ const LeadingVisual: React.FC> = ({chil ( { - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), alignItems: 'center', height: titleAreaHeight, }, @@ -272,7 +272,7 @@ const Title: React.FC> = ({children, sx = {} medium: '600', subtitle: '400', }[titleVariant], - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), }, sx, )} @@ -288,7 +288,7 @@ const TrailingVisual: React.FC> = ({chi ( { - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), alignItems: 'center', height: titleAreaHeight, }, @@ -311,7 +311,7 @@ const TrailingAction: React.FC> = ({ ( { - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), alignItems: 'center', height: titleAreaHeight, }, @@ -329,7 +329,7 @@ const Actions: React.FC> = ({children, ( { - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), flexDirection: 'row', gap: '0.5rem', flexGrow: '1', @@ -351,7 +351,7 @@ const Description: React.FC> = ({childr ( { - ...displayResponsively(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), flexDirection: 'row', alignItems: 'center', gap: '0.5rem', @@ -367,7 +367,7 @@ 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}) => { return ( - (displayResponsively(hidden, false, 'display', 'none', 'block'), sx)}> + (CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'block'), sx)}> {children} ) diff --git a/src/PageHeader/manageResponsiveVisibility.ts b/src/PageHeader/manageResponsiveValueWithCSS.ts similarity index 54% rename from src/PageHeader/manageResponsiveVisibility.ts rename to src/PageHeader/manageResponsiveValueWithCSS.ts index 9d7f11fd81b..712b0b3603d 100644 --- a/src/PageHeader/manageResponsiveVisibility.ts +++ b/src/PageHeader/manageResponsiveValueWithCSS.ts @@ -1,8 +1,30 @@ import {isResponsiveValue, ResponsiveValue} from '../hooks/useResponsiveValue' import {BetterSystemStyleObject} from '../sx' -export function displayResponsively( - // what is your value that you want to render responsivley? - responsiveValue +/** + * This function is inspired by the `useResponsiveValue` hook and it's used to render responsive values with CSS. + * @param value - The value that we want to render responsively + * @param fallback - The fallback value + * @param cssProperty - The CSS property that we want to render + * @param truety - The value that we want to render when the value is true + * @param falsy - The value that we want to render when the value is false + + * @example + * CSSManagedResponsiveValue({narrow: true, regular: true, wide: false}, false, 'display', '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" + * } + * } + */ +export function CSSManagedResponsiveValue( value: T, fallback: F, cssProperty: string, @@ -14,12 +36,6 @@ export function displayResponsively( const responsiveValue = value as Extract> const checkFallback = fallback ? truety : falsy - // arguments - // what is the value that we want to render responsively? - responsiveValue - // what is your css property? - display - // truety value - none - // falsy value - flex - return { [`@media screen and (max-width: 768px)`]: { [cssProperty]: 'narrow' in responsiveValue ? (responsiveValue.narrow ? truety : falsy) : checkFallback, From d311f6ff80e527ff6efbc33323ffb108ac9b3629 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Mon, 12 Dec 2022 14:50:54 +1000 Subject: [PATCH 04/12] update unit tests --- src/PageHeader/PageHeader.test.tsx | 133 ++++++++++++++++++----------- 1 file changed, 83 insertions(+), 50 deletions(-) diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx index 57a16150342..9f8ab9d0def 100644 --- a/src/PageHeader/PageHeader.test.tsx +++ b/src/PageHeader/PageHeader.test.tsx @@ -3,7 +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 {behavesAsComponent, checkExports, checkStoriesForAxeViolations, renderStyles} from '../utils/testing' import {act} from 'react-test-renderer' import {viewportRanges} from '../hooks/useResponsiveValue' import {IconButton} from '../Button' @@ -44,29 +44,64 @@ describe('PageHeader', () => { ) expect(container).toMatchSnapshot() }) - it.skip('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 quesry 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:768px)`]: { + display: 'flex', + }, + [`@media screen and (min-width:768px)`]: { + display: 'none', + }, + [`@media screen and (min-width:1440px)`]: { + display: 'none', + }, + 'align-items': 'center', + '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')).toHaveStyle('display: none') - // expect(getByText('ContextArea')).not.toBeVisible() }) - it.skip('respects the hidden prop of ContextArea and renders accordingly', () => { - act(() => { - matchmedia.useMediaQuery(viewportRanges.wide) - }) + 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:768px)`]: { + display: 'flex', + }, + [`@media screen and (min-width:768px)`]: { + display: 'flex', + }, + [`@media screen and (min-width:1440px)`]: { + display: 'none', + }, + 'align-items': 'center', + 'flex-direction': 'row', + gap: '0.5rem', + order: '0', + } - const {getByText} = render( - + expect( + renderStyles( - TitleArea - Description - Navigation - , - ) - expect(getByText('ContextArea')).not.toBeVisible() - expect(getByText('ContextArea')).toHaveStyle('display: none') + , + ), + ).toEqual(expect.objectContaining(expectedStyles)) }) - it.skip('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:768px)`]: { + display: 'none', + }, + [`@media screen and (min-width:768px)`]: { + display: 'flex', + }, + [`@media screen and (min-width:1440px)`]: { + display: 'flex', + }, + 'align-items': 'center', + height: '2rem', + } + + expect( + renderStyles( + + + , + ), + ).toEqual(expect.objectContaining(expectedStyles)) }) it('respects the title variant prop', () => { const {getByText} = render( From 6264903cbf687cd4ff351a512a2206c621a8b4e3 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Mon, 12 Dec 2022 14:53:06 +1000 Subject: [PATCH 05/12] add changeset --- .changeset/polite-bats-confess.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/polite-bats-confess.md diff --git a/.changeset/polite-bats-confess.md b/.changeset/polite-bats-confess.md new file mode 100644 index 00000000000..7f1c90f6e2b --- /dev/null +++ b/.changeset/polite-bats-confess.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +PageHeader: Manage responsive visibility with CSS breakpoints From af0cdb3035ed066f175c89b2560faa9684cac36b Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Mon, 12 Dec 2022 18:53:55 +1000 Subject: [PATCH 06/12] remove unused imports --- src/PageHeader/PageHeader.test.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx index 9f8ab9d0def..a66f1d0fe9e 100644 --- a/src/PageHeader/PageHeader.test.tsx +++ b/src/PageHeader/PageHeader.test.tsx @@ -4,8 +4,6 @@ import {render} from '@testing-library/react' import {PageHeader} from '.' import MatchMediaMock from 'jest-matchmedia-mock' import {behavesAsComponent, checkExports, checkStoriesForAxeViolations, renderStyles} from '../utils/testing' -import {act} from 'react-test-renderer' -import {viewportRanges} from '../hooks/useResponsiveValue' import {IconButton} from '../Button' import {ChevronLeftIcon, GitBranchIcon, PencilIcon, SidebarExpandIcon} from '@primer/octicons-react' From b561e9e6d51cd23922b1629749b773886ee20e1f Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 14 Dec 2022 14:19:01 +1000 Subject: [PATCH 07/12] address code review feedback --- src/PageHeader/PageHeader.test.tsx | 6 - src/PageHeader/PageHeader.tsx | 84 ++++++++++-- .../__snapshots__/PageHeader.test.tsx.snap | 12 -- .../manageResponsiveValueWithCSS.ts | 121 ++++++++++++++---- 4 files changed, 170 insertions(+), 53 deletions(-) diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx index a66f1d0fe9e..1d848b7efdb 100644 --- a/src/PageHeader/PageHeader.test.tsx +++ b/src/PageHeader/PageHeader.test.tsx @@ -61,9 +61,6 @@ describe('PageHeader', () => { [`@media screen and (min-width:768px)`]: { display: 'none', }, - [`@media screen and (min-width:1440px)`]: { - display: 'none', - }, 'align-items': 'center', 'flex-direction': 'row', gap: '0.5rem', @@ -123,9 +120,6 @@ describe('PageHeader', () => { [`@media screen and (min-width:768px)`]: { display: 'flex', }, - [`@media screen and (min-width:1440px)`]: { - display: 'flex', - }, 'align-items': 'center', height: '2rem', } diff --git a/src/PageHeader/PageHeader.tsx b/src/PageHeader/PageHeader.tsx index e3e24629d60..4e68ac9443b 100644 --- a/src/PageHeader/PageHeader.tsx +++ b/src/PageHeader/PageHeader.tsx @@ -63,13 +63,37 @@ const ContextArea: React.FC> = ({ hidden = hiddenOnRegularAndWide, sx = {}, }) => { + // const mediaQueryStyles = merge( + // CSSManagedResponsiveValue(hidden, 'display', value => { + // return value ? 'none' : 'flex' + // }), + // CSSManagedResponsiveValue( + // { + // narrow: 'none', + // regular: 'line', + // wide: 'filled', + // }, + // 'backgroundColor', + // (value): string => { + // return { + // none: 'pink', + // line: 'salmon', + // filled: 'blue', + // }[value] + // }, + // ), + // ) + const contentNavStyles = { flexDirection: 'row', alignItems: 'center', gap: '0.5rem', order: REGION_ORDER.ContextArea, - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), } + return (contentNavStyles, sx)}>{children} } type LinkProps = Pick< @@ -101,7 +125,9 @@ const ParentLink = React.forwardRef( { alignItems: 'center', gap: '0.5rem', - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), }, sx, )} @@ -125,7 +151,14 @@ const ContextBar: React.FC> = ({ hidden = hiddenOnRegularAndWide, }) => { return ( - (CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), sx)}> + ( + CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), + sx, + )} + > {children} ) @@ -147,7 +180,9 @@ const ContextAreaActions: React.FC> = ( gap: '0.5rem', flexGrow: '1', justifyContent: 'right', - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), }, sx, )} @@ -190,7 +225,9 @@ const TitleArea: React.FC> = ({ sx={merge( { gap: '0.5rem', - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), flexDirection: 'row', alignItems: 'flex-start', }, @@ -214,7 +251,9 @@ const LeadingAction: React.FC> = ({ ( { - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), alignItems: 'center', height: titleAreaHeight, }, @@ -232,7 +271,9 @@ const LeadingVisual: React.FC> = ({chil ( { - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), alignItems: 'center', height: titleAreaHeight, }, @@ -272,7 +313,9 @@ const Title: React.FC> = ({children, sx = {} medium: '600', subtitle: '400', }[titleVariant], - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), }, sx, )} @@ -288,7 +331,9 @@ const TrailingVisual: React.FC> = ({chi ( { - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), alignItems: 'center', height: titleAreaHeight, }, @@ -311,7 +356,9 @@ const TrailingAction: React.FC> = ({ ( { - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), alignItems: 'center', height: titleAreaHeight, }, @@ -329,7 +376,9 @@ const Actions: React.FC> = ({children, ( { - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), flexDirection: 'row', gap: '0.5rem', flexGrow: '1', @@ -351,7 +400,9 @@ const Description: React.FC> = ({childr ( { - ...CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'flex'), + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), flexDirection: 'row', alignItems: 'center', gap: '0.5rem', @@ -367,7 +418,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}) => { return ( - (CSSManagedResponsiveValue(hidden, false, 'display', 'none', 'block'), sx)}> + ( + CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'block' + }), + sx, + )} + > {children} ) diff --git a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap index 3de7753c0a5..ea05a8e6a12 100644 --- a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap +++ b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap @@ -75,12 +75,6 @@ exports[`PageHeader renders consistently 1`] = ` } } -@media screen and (min-width:1440px) { - .c1 { - display: none; - } -} -
@@ -174,12 +168,6 @@ exports[`PageHeader renders default layout 1`] = ` } } -@media screen and (min-width:1440px) { - .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 we want to render responsively - * @param fallback - The fallback value - * @param cssProperty - The CSS property that we want to render - * @param truety - The value that we want to render when the value is true - * @param falsy - The value that we want to render when the value is false + * @param value - The value that needs to be rendered responsively + * @param cssProperty - The CSS property whoes value needs to be rendered responsively + * @param comparisonFunc - A function that evaluates which value to assign to the CSS property * @example - * CSSManagedResponsiveValue({narrow: true, regular: true, wide: false}, false, 'display', 'none', 'flex') + * CSSManagedResponsiveValue({narrow: true, regular: true, wide: false}, 'display', value => { + return value ? 'none' : 'flex' + }) * @returns * { * "@media screen and (max-width: 768px)": { @@ -23,31 +37,94 @@ import {BetterSystemStyleObject} from '../sx' * "display": "flex" * } * } + * + * * @example + * CSSManagedResponsiveValue({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 +* CSSManagedResponsiveValue({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 + * }, + * } */ -export function CSSManagedResponsiveValue( +export function CSSManagedResponsiveValue( value: T, - fallback: F, cssProperty: string, - truety: B, - falsy: C, + comparisonFunc: (value: T) => void, ): BetterSystemStyleObject { if (isResponsiveValue(value)) { // eslint-disable-next-line @typescript-eslint/no-explicit-any const responsiveValue = value as Extract> - const checkFallback = fallback ? truety : falsy - return { - [`@media screen and (max-width: 768px)`]: { - [cssProperty]: 'narrow' in responsiveValue ? (responsiveValue.narrow ? truety : falsy) : checkFallback, - }, - [`@media screen and (min-width: 768px)`]: { - [cssProperty]: 'regular' in responsiveValue ? (responsiveValue.regular ? truety : falsy) : checkFallback, - }, - [`@media screen and (min-width: 1440px)`]: { - [cssProperty]: 'wide' in responsiveValue ? (responsiveValue.wide ? truety : falsy) : checkFallback, - }, + // Build media queries with the giving cssProperty and comparisonFunc + const narrowMediaQuery = + 'narrow' in responsiveValue + ? { + [`@media screen and (max-width: 768px)`]: { + [cssProperty]: comparisonFunc(responsiveValue.narrow), + }, + } + : {} + + const regularMediaQuery = + 'regular' in responsiveValue + ? { + [`@media screen and (min-width: 768px)`]: { + [cssProperty]: comparisonFunc(responsiveValue.regular), + }, + } + : {} + + const wideMediaQuery = + 'wide' in responsiveValue + ? { + [`@media screen and (min-width: 1440px)`]: { + [cssProperty]: comparisonFunc(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]: comparisonFunc(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 { - return {[cssProperty]: value ? truety : falsy} + // If the given value is not a responsive value + return {[cssProperty]: comparisonFunc(value)} } } From 415dbf84ba5da64d95ebd3035a8d0b474ac95f71 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 15 Dec 2022 14:13:44 +1000 Subject: [PATCH 08/12] type fixes & small adj & add fallback CSS declaration --- src/PageHeader/PageHeader.tsx | 29 ++++++++++++---- .../manageResponsiveValueWithCSS.ts | 33 ++++++++++--------- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/PageHeader/PageHeader.tsx b/src/PageHeader/PageHeader.tsx index 4e68ac9443b..39626dd48ad 100644 --- a/src/PageHeader/PageHeader.tsx +++ b/src/PageHeader/PageHeader.tsx @@ -85,6 +85,7 @@ const ContextArea: React.FC> = ({ // ) const contentNavStyles = { + display: 'flex', flexDirection: 'row', alignItems: 'center', gap: '0.5rem', @@ -123,6 +124,7 @@ const ParentLink = React.forwardRef( muted sx={merge( { + display: 'flex', alignItems: 'center', gap: '0.5rem', ...CSSManagedResponsiveValue(hidden, 'display', value => { @@ -153,9 +155,12 @@ const ContextBar: React.FC> = ({ return ( ( - CSSManagedResponsiveValue(hidden, 'display', value => { - return value ? 'none' : 'flex' - }), + { + display: 'flex', + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'flex' + }), + }, sx, )} > @@ -175,6 +180,7 @@ const ContextAreaActions: React.FC> = ( ( { + display: 'flex', flexDirection: 'row', alignItems: 'center', gap: '0.5rem', @@ -224,6 +230,7 @@ const TitleArea: React.FC> = ({ ( { + display: 'flex', gap: '0.5rem', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' @@ -251,6 +258,7 @@ const LeadingAction: React.FC> = ({ ( { + display: 'flex', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' }), @@ -271,6 +279,7 @@ const LeadingVisual: React.FC> = ({chil ( { + display: 'flex', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' }), @@ -313,6 +322,7 @@ const Title: React.FC> = ({children, sx = {} medium: '600', subtitle: '400', }[titleVariant], + display: 'flex', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' }), @@ -331,6 +341,7 @@ const TrailingVisual: React.FC> = ({chi ( { + display: 'flex', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' }), @@ -356,6 +367,7 @@ const TrailingAction: React.FC> = ({ ( { + display: 'flex', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' }), @@ -376,6 +388,7 @@ const Actions: React.FC> = ({children, ( { + display: 'flex', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' }), @@ -400,6 +413,7 @@ const Description: React.FC> = ({childr ( { + display: 'flex', ...CSSManagedResponsiveValue(hidden, 'display', value => { return value ? 'none' : 'flex' }), @@ -420,9 +434,12 @@ const Navigation: React.FC> = ({childre return ( ( - CSSManagedResponsiveValue(hidden, 'display', value => { - return value ? 'none' : 'block' - }), + { + display: 'flex', + ...CSSManagedResponsiveValue(hidden, 'display', value => { + return value ? 'none' : 'block' + }), + }, sx, )} > diff --git a/src/PageHeader/manageResponsiveValueWithCSS.ts b/src/PageHeader/manageResponsiveValueWithCSS.ts index 5fa034d6cd5..204ded0ce4e 100644 --- a/src/PageHeader/manageResponsiveValueWithCSS.ts +++ b/src/PageHeader/manageResponsiveValueWithCSS.ts @@ -1,5 +1,6 @@ -import {isResponsiveValue, ResponsiveValue} from '../hooks/useResponsiveValue' +import {isResponsiveValue, ResponsiveValue, viewportRanges} from '../hooks/useResponsiveValue' import {BetterSystemStyleObject} from '../sx' +import * as CSS from 'csstype' function areAllValuesTheSame(responsiveValue: ResponsiveValue): boolean { if ('narrow' in responsiveValue && 'regular' in responsiveValue && 'wide' in responsiveValue) { @@ -19,7 +20,7 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue { @@ -69,21 +70,21 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue( - value: T, - cssProperty: string, - comparisonFunc: (value: T) => void, +export function CSSManagedResponsiveValue( + value: TInput | ResponsiveValue, + cssProperty: keyof CSS.Properties, + mapFn: (value: TInput) => TOutput, ): BetterSystemStyleObject { if (isResponsiveValue(value)) { // eslint-disable-next-line @typescript-eslint/no-explicit-any - const responsiveValue = value as Extract> + const responsiveValue = value as Extract> - // Build media queries with the giving cssProperty and comparisonFunc + // Build media queries with the giving cssProperty and mapFn const narrowMediaQuery = 'narrow' in responsiveValue ? { - [`@media screen and (max-width: 768px)`]: { - [cssProperty]: comparisonFunc(responsiveValue.narrow), + [`@media screen and ${viewportRanges.narrow}`]: { + [cssProperty]: mapFn(responsiveValue.narrow), }, } : {} @@ -91,8 +92,8 @@ export function CSSManagedResponsiveValue( const regularMediaQuery = 'regular' in responsiveValue ? { - [`@media screen and (min-width: 768px)`]: { - [cssProperty]: comparisonFunc(responsiveValue.regular), + [`@media screen and ${viewportRanges.regular}`]: { + [cssProperty]: mapFn(responsiveValue.regular), }, } : {} @@ -100,8 +101,8 @@ export function CSSManagedResponsiveValue( const wideMediaQuery = 'wide' in responsiveValue ? { - [`@media screen and (min-width: 1440px)`]: { - [cssProperty]: comparisonFunc(responsiveValue.wide), + [`@media screen and ${viewportRanges.wide}`]: { + [cssProperty]: mapFn(responsiveValue.wide), }, } : {} @@ -109,7 +110,7 @@ export function CSSManagedResponsiveValue( // 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]: comparisonFunc(responsiveValue.narrow)} + 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 { @@ -125,6 +126,6 @@ export function CSSManagedResponsiveValue( } } else { // If the given value is not a responsive value - return {[cssProperty]: comparisonFunc(value)} + return {[cssProperty]: mapFn(value)} } } From 3a7dde69729f5f7c21b9432374f77bd8430a511b Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 15 Dec 2022 14:57:59 +1000 Subject: [PATCH 09/12] type fix and add comment --- src/PageHeader/manageResponsiveValueWithCSS.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/PageHeader/manageResponsiveValueWithCSS.ts b/src/PageHeader/manageResponsiveValueWithCSS.ts index 204ded0ce4e..ad339bd6cdc 100644 --- a/src/PageHeader/manageResponsiveValueWithCSS.ts +++ b/src/PageHeader/manageResponsiveValueWithCSS.ts @@ -1,6 +1,6 @@ import {isResponsiveValue, ResponsiveValue, viewportRanges} from '../hooks/useResponsiveValue' import {BetterSystemStyleObject} from '../sx' -import * as CSS from 'csstype' +import type {Properties as CSSProperties} from 'csstype' function areAllValuesTheSame(responsiveValue: ResponsiveValue): boolean { if ('narrow' in responsiveValue && 'regular' in responsiveValue && 'wide' in responsiveValue) { @@ -20,7 +20,10 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue { @@ -72,7 +75,7 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue( value: TInput | ResponsiveValue, - cssProperty: keyof CSS.Properties, + cssProperty: keyof CSSProperties, mapFn: (value: TInput) => TOutput, ): BetterSystemStyleObject { if (isResponsiveValue(value)) { From bb6639b35bdfe4579539b8422bd8e9d61645cc56 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 15 Dec 2022 15:08:12 +1000 Subject: [PATCH 10/12] update tests --- src/PageHeader/PageHeader.test.tsx | 10 ++++++---- .../__snapshots__/PageHeader.test.tsx.snap | 16 ++++++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx index 1d848b7efdb..8f661e45234 100644 --- a/src/PageHeader/PageHeader.test.tsx +++ b/src/PageHeader/PageHeader.test.tsx @@ -55,13 +55,14 @@ describe('PageHeader', () => { '-webkit-box-align': 'center', '-webkit-flex-direction': 'row', '-webkit-order': '0', - [`@media screen and (max-width:768px)`]: { + [`@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', @@ -80,16 +81,17 @@ describe('PageHeader', () => { '-webkit-box-align': 'center', '-webkit-flex-direction': 'row', '-webkit-order': '0', - [`@media screen and (max-width:768px)`]: { + [`@media screen and (max-width:calc(768px - 0.02px))`]: { display: 'flex', }, [`@media screen and (min-width:768px)`]: { display: 'flex', }, - [`@media screen and (min-width:1440px)`]: { + [`@media screen and (min-width:1400px)`]: { display: 'none', }, 'align-items': 'center', + display: 'flex', 'flex-direction': 'row', gap: '0.5rem', order: '0', @@ -114,7 +116,7 @@ describe('PageHeader', () => { '-ms-flex-align': 'center', '-webkit-align-items': 'center', '-webkit-box-align': 'center', - [`@media screen and (max-width:768px)`]: { + [`@media screen and (max-width:calc(768px - 0.02px))`]: { display: 'none', }, [`@media screen and (min-width:768px)`]: { diff --git a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap index ea05a8e6a12..25f75b6d9b1 100644 --- a/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap +++ b/src/PageHeader/__snapshots__/PageHeader.test.tsx.snap @@ -13,6 +13,10 @@ exports[`PageHeader renders consistently 1`] = ` } .c1 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; -webkit-flex-direction: row; -ms-flex-direction: row; flex-direction: row; @@ -27,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; @@ -60,7 +64,7 @@ exports[`PageHeader renders consistently 1`] = ` display: block; } -@media screen and (max-width:768px) { +@media screen and (max-width:calc(768px - 0.02px)) { .c1 { display: -webkit-box; display: -webkit-flex; @@ -106,6 +110,10 @@ exports[`PageHeader renders default layout 1`] = ` } .c1 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; -webkit-flex-direction: row; -ms-flex-direction: row; flex-direction: row; @@ -120,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; @@ -153,7 +161,7 @@ exports[`PageHeader renders default layout 1`] = ` display: block; } -@media screen and (max-width:768px) { +@media screen and (max-width:calc(768px - 0.02px)) { .c1 { display: -webkit-box; display: -webkit-flex; From d3eb78a09cd6242c116c3b1d31211c8b7e845d3a Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 15 Dec 2022 20:40:55 +1000 Subject: [PATCH 11/12] update changeset description --- .changeset/polite-bats-confess.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/polite-bats-confess.md b/.changeset/polite-bats-confess.md index 7f1c90f6e2b..849260b1faf 100644 --- a/.changeset/polite-bats-confess.md +++ b/.changeset/polite-bats-confess.md @@ -2,4 +2,4 @@ '@primer/react': minor --- -PageHeader: Manage responsive visibility with CSS breakpoints +PageHeader: Add a util function that returns breakpoint styles with the given CSS property and values From 0927bd1e21128ce7ca211a8dbc398447a89408d1 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 28 Dec 2022 12:06:43 +1000 Subject: [PATCH 12/12] rename the function and move it into utils/ --- src/PageHeader/PageHeader.test.tsx | 2 +- src/PageHeader/PageHeader.tsx | 49 ++++++------------- .../getBreakpointDeclarations.ts} | 41 +++++++++++++--- 3 files changed, 49 insertions(+), 43 deletions(-) rename src/{PageHeader/manageResponsiveValueWithCSS.ts => utils/getBreakpointDeclarations.ts} (76%) diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx index 8f661e45234..4e0cf753526 100644 --- a/src/PageHeader/PageHeader.test.tsx +++ b/src/PageHeader/PageHeader.test.tsx @@ -44,7 +44,7 @@ describe('PageHeader', () => { }) /** 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 quesry testing with Jest. + * 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 = { diff --git a/src/PageHeader/PageHeader.tsx b/src/PageHeader/PageHeader.tsx index 39626dd48ad..ca7e9e8348c 100644 --- a/src/PageHeader/PageHeader.tsx +++ b/src/PageHeader/PageHeader.tsx @@ -6,7 +6,7 @@ import Heading from '../Heading' import {ArrowLeftIcon} from '@primer/octicons-react' import Link from '../Link' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' -import {CSSManagedResponsiveValue} from './manageResponsiveValueWithCSS' +import {getBreakpointDeclarations} from '../utils/getBreakpointDeclarations' const REGION_ORDER = { ContextArea: 0, TitleArea: 1, @@ -63,34 +63,13 @@ const ContextArea: React.FC> = ({ hidden = hiddenOnRegularAndWide, sx = {}, }) => { - // const mediaQueryStyles = merge( - // CSSManagedResponsiveValue(hidden, 'display', value => { - // return value ? 'none' : 'flex' - // }), - // CSSManagedResponsiveValue( - // { - // narrow: 'none', - // regular: 'line', - // wide: 'filled', - // }, - // 'backgroundColor', - // (value): string => { - // return { - // none: 'pink', - // line: 'salmon', - // filled: 'blue', - // }[value] - // }, - // ), - // ) - const contentNavStyles = { display: 'flex', flexDirection: 'row', alignItems: 'center', gap: '0.5rem', order: REGION_ORDER.ContextArea, - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), } @@ -127,7 +106,7 @@ const ParentLink = React.forwardRef( display: 'flex', alignItems: 'center', gap: '0.5rem', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), }, @@ -157,7 +136,7 @@ const ContextBar: React.FC> = ({ sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), }, @@ -186,7 +165,7 @@ const ContextAreaActions: React.FC> = ( gap: '0.5rem', flexGrow: '1', justifyContent: 'right', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), }, @@ -232,7 +211,7 @@ const TitleArea: React.FC> = ({ { display: 'flex', gap: '0.5rem', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), flexDirection: 'row', @@ -259,7 +238,7 @@ const LeadingAction: React.FC> = ({ sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), alignItems: 'center', @@ -280,7 +259,7 @@ const LeadingVisual: React.FC> = ({chil sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), alignItems: 'center', @@ -323,7 +302,7 @@ const Title: React.FC> = ({children, sx = {} subtitle: '400', }[titleVariant], display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), }, @@ -342,7 +321,7 @@ const TrailingVisual: React.FC> = ({chi sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), alignItems: 'center', @@ -368,7 +347,7 @@ const TrailingAction: React.FC> = ({ sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), alignItems: 'center', @@ -389,7 +368,7 @@ const Actions: React.FC> = ({children, sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), flexDirection: 'row', @@ -414,7 +393,7 @@ const Description: React.FC> = ({childr sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'flex' }), flexDirection: 'row', @@ -436,7 +415,7 @@ const Navigation: React.FC> = ({childre sx={merge( { display: 'flex', - ...CSSManagedResponsiveValue(hidden, 'display', value => { + ...getBreakpointDeclarations(hidden, 'display', value => { return value ? 'none' : 'block' }), }, diff --git a/src/PageHeader/manageResponsiveValueWithCSS.ts b/src/utils/getBreakpointDeclarations.ts similarity index 76% rename from src/PageHeader/manageResponsiveValueWithCSS.ts rename to src/utils/getBreakpointDeclarations.ts index ad339bd6cdc..4076ab638c5 100644 --- a/src/PageHeader/manageResponsiveValueWithCSS.ts +++ b/src/utils/getBreakpointDeclarations.ts @@ -22,11 +22,13 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue { + * getBreakpointDeclarations({narrow: true, regular: true, wide: false}, 'display', value => { return value ? 'none' : 'flex' }) * @returns @@ -43,7 +45,7 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue { + * getBreakpointDeclarations({regular: 'border.default', wide: 'canvas.inset'}, 'backgroundColor', (value): string => { return value }) * @returns @@ -57,7 +59,7 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue { +* getBreakpointDeclarations({narrow: 'filled', regular: 'line'}, 'height', (value): string => { return { filled: 8, line: 1, @@ -72,8 +74,33 @@ function haveRegularAndWideSameValue(responsiveValue: ResponsiveValue( + 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 CSSManagedResponsiveValue( +export function getBreakpointDeclarations( value: TInput | ResponsiveValue, cssProperty: keyof CSSProperties, mapFn: (value: TInput) => TOutput,