diff --git a/.changeset/plenty-friends-flash.md b/.changeset/plenty-friends-flash.md new file mode 100644 index 00000000000..5d469493a10 --- /dev/null +++ b/.changeset/plenty-friends-flash.md @@ -0,0 +1,5 @@ +--- +'@primer/react': major +--- + +Removes sx prop from PageHeader and subcomponents diff --git a/packages/react/src/PageHeader/PageHeader.tsx b/packages/react/src/PageHeader/PageHeader.tsx index f78a6890b12..9fe8a7bd9a3 100644 --- a/packages/react/src/PageHeader/PageHeader.tsx +++ b/packages/react/src/PageHeader/PageHeader.tsx @@ -7,7 +7,7 @@ import {ArrowLeftIcon} from '@primer/octicons-react' import type {LinkProps as BaseLinkProps} from '../Link' import Link from '../Link' -import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {fixedForwardRef, type PolymorphicProps} from '../utils/modern-polymorphic' import {areAllValuesTheSame, haveRegularAndWideSameValue} from '../utils/getBreakpointDeclarations' import {warning} from '../utils/warning' import {useProvidedRefOrCreate} from '../hooks' @@ -48,77 +48,93 @@ export type PageHeaderProps = { hasBorder?: boolean } & SxProp -const Root = React.forwardRef>( - ({children, className, sx = defaultSxProp, as = 'div', 'aria-label': ariaLabel, role, hasBorder}, forwardedRef) => { - const rootRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) - - const isInteractive = (element: HTMLElement) => { - return ( - ['a', 'button'].some(selector => element.matches(selector)) || - (element.hasAttribute('role') && element.getAttribute('role') === 'button') || - (element.hasAttribute('link') && element.getAttribute('role') === 'link') || - element.hasAttribute('tabindex') - ) - } +function UnwrappedRoot( + props: PolymorphicProps & PageHeaderProps, + forwardedRef: React.ForwardedRef, +) { + const { + children, + className, + sx = defaultSxProp, + as = 'div', + 'aria-label': ariaLabel, + role, + hasBorder, + ...restProps + } = props + const rootRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + + const isInteractive = (element: HTMLElement) => { + return ( + ['a', 'button'].some(selector => element.matches(selector)) || + (element.hasAttribute('role') && element.getAttribute('role') === 'button') || + (element.hasAttribute('link') && element.getAttribute('role') === 'link') || + element.hasAttribute('tabindex') + ) + } - useEffect( - function validateInteractiveElementsInTitle() { - if (!__DEV__) return + useEffect( + function validateInteractiveElementsInTitle() { + if (!__DEV__) return - let hasContextArea = false - let hasLeadingAction = false + let hasContextArea = false + let hasLeadingAction = false - if (!rootRef.current || rootRef.current.children.length <= 0) return - const titleArea = Array.from(rootRef.current.children as HTMLCollection).find(child => { - return child instanceof HTMLElement && child.getAttribute('data-component') === 'TitleArea' - }) + if (!rootRef.current || rootRef.current.children.length <= 0) return + const titleArea = Array.from(rootRef.current.children as HTMLCollection).find(child => { + return child instanceof HTMLElement && child.getAttribute('data-component') === 'TitleArea' + }) - // It is very unlikely to have a PageHeader without a TitleArea, but we still want to make sure we don't break the page if that happens. - if (!titleArea) return + // It is very unlikely to have a PageHeader without a TitleArea, but we still want to make sure we don't break the page if that happens. + if (!titleArea) return - for (const child of React.Children.toArray(children)) { - if (React.isValidElement(child) && child.type === ContextArea) { - hasContextArea = true - } - if (React.isValidElement(child) && child.type === LeadingAction) { - hasLeadingAction = true - } + for (const child of React.Children.toArray(children)) { + if (React.isValidElement(child) && child.type === ContextArea) { + hasContextArea = true } - // Check if TitleArea has any interactive children or grandchildren. - const hasInteractiveContent = Array.from(titleArea.childNodes).some(child => { - return ( - (child instanceof HTMLElement && isInteractive(child)) || - Array.from(child.childNodes).some(child => { - return child instanceof HTMLElement && isInteractive(child) - }) - ) - }) - // PageHeader.TitleArea is be the first element in the DOM even when it is not visually the first. - // Motivation behind this rule to make sure context area and leading action (if they exist) are always rendered after the title (a heading tag) - // so that screen reader users who are navigating via heading menu won't miss these actions. - warning( - hasInteractiveContent && (hasContextArea || hasLeadingAction), - 'When PageHeader.ContextArea or PageHeader.LeadingAction is present, we recommended not to include any interactive items in the PageHeader.TitleArea to make sure the focus order is logical.', + if (React.isValidElement(child) && child.type === LeadingAction) { + hasLeadingAction = true + } + } + // Check if TitleArea has any interactive children or grandchildren. + const hasInteractiveContent = Array.from(titleArea.childNodes).some(child => { + return ( + (child instanceof HTMLElement && isInteractive(child)) || + Array.from(child.childNodes).some(child => { + return child instanceof HTMLElement && isInteractive(child) + }) ) - }, - [children, rootRef], - ) + }) + // PageHeader.TitleArea is be the first element in the DOM even when it is not visually the first. + // Motivation behind this rule to make sure context area and leading action (if they exist) are always rendered after the title (a heading tag) + // so that screen reader users who are navigating via heading menu won't miss these actions. + warning( + hasInteractiveContent && (hasContextArea || hasLeadingAction), + 'When PageHeader.ContextArea or PageHeader.LeadingAction is present, we recommended not to include any interactive items in the PageHeader.TitleArea to make sure the focus order is logical.', + ) + }, + [children, rootRef], + ) - return ( - - {children} - - ) - }, -) as PolymorphicForwardRefComponent<'div', PageHeaderProps> + return ( + + {children} + + ) +} + +const Root = fixedForwardRef(UnwrappedRoot) + +Object.assign(Root, {displayName: 'PageHeader'}) // PageHeader.ContextArea : Only visible on narrow viewports by default to provide user context of where they are at their journey. `hidden` prop available // to manage their custom visibility but consumers should be careful if they choose to hide this on narrow viewports. @@ -136,48 +152,42 @@ const ContextArea: React.FC> = ({ ) } -type LinkProps = Pick< +type LinkProps = Pick< React.AnchorHTMLAttributes & BaseLinkProps, 'download' | 'href' | 'hrefLang' | 'media' | 'ping' | 'rel' | 'target' | 'type' | 'referrerPolicy' | 'as' > & { 'aria-label'?: React.AriaAttributes['aria-label'] + as?: As } export type ParentLinkProps = React.PropsWithChildren -// PageHeader.ParentLink : Only visible on narrow viewports by default to let users navigate up in the hierarchy. -const ParentLink = React.forwardRef( - ( - { - children, - className, - sx: sxProp = defaultSxProp, - href, - 'aria-label': ariaLabel, - as = 'a', - hidden = hiddenOnRegularAndWide, - }, - ref, - ) => { - return ( - <> - - -
{children}
- - - ) - }, -) as PolymorphicForwardRefComponent<'a', ParentLinkProps> -ParentLink.displayName = 'ParentLink' +function UnwrappedParentLink( + props: React.PropsWithChildren>, + forwardedRef: React.ForwardedRef, +) { + const {children, className, 'aria-label': ariaLabel, as = 'a', hidden = hiddenOnRegularAndWide, ...restProps} = props + + return ( + <> + } + as={as} + aria-label={ariaLabel} + muted + className={clsx(classes.ParentLink, className)} + {...getHiddenDataAttributes(hidden)} + {...restProps} + > + +
{children}
+ + + ) +} + +const ParentLink = fixedForwardRef(UnwrappedParentLink) + +Object.assign(ParentLink, {displayName: 'ParentLink'}) // ContextBar // Generic slot for any component above the title region. Use it for custom breadcrumbs and other navigation elements instead of ParentLink. @@ -223,25 +233,31 @@ type TitleAreaProps = { // PageHeader.TitleArea Sub Components: PageHeader.LeadingVisual, PageHeader.Title, PageTitle.TrailingVisual // --------------------------------------------------------------------- -const TitleArea = React.forwardRef>( - ({children, className, sx: sxProp = defaultSxProp, hidden = false, variant = 'medium'}, forwardedRef) => { - const titleAreaRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) - const currentVariant = useResponsiveValue(variant, 'medium') - return ( - - {children} - - ) - }, -) as PolymorphicForwardRefComponent<'div', TitleAreaProps> -TitleArea.displayName = 'TitleArea' +function UnwrappedTitleArea( + props: React.PropsWithChildren>, + forwardedRef: React.ForwardedRef, +) { + const {children, className, sx: sxProp = defaultSxProp, hidden = false, variant = 'medium', ...restProps} = props + const titleAreaRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const currentVariant = useResponsiveValue(variant, 'medium') + return ( + + {children} + + ) +} + +const TitleArea = fixedForwardRef(UnwrappedTitleArea) + +Object.assign(TitleArea, {displayName: 'PageHeader.TitleArea'}) // PageHeader.LeadingAction and PageHeader.TrailingAction should only be visible on regular viewports. // So they come as hidden on narrow viewports by default and their visibility can be managed by their `hidden` prop. @@ -336,7 +352,6 @@ const Title: React.FC> = ({ data-hidden={hidden} as={as} style={style} - sx={sxProp} {...getHiddenDataAttributes(hidden)} > {children} @@ -537,5 +552,3 @@ export const PageHeader = Object.assign(Root, { Description, Navigation, }) - -PageHeader.displayName = 'PageHeader' diff --git a/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap b/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap index d34f9dce59c..5b2c5fb1549 100644 --- a/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap +++ b/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap @@ -112,6 +112,7 @@ exports[`@primer/react > should not update exports without a semver change 1`] = "type PageLayoutProps", "Pagination", "type PaginationProps", + "type ParentLinkProps", "PointerBox", "type PointerBoxProps", "Popover", @@ -183,6 +184,7 @@ exports[`@primer/react > should not update exports without a semver change 1`] = "type TimelineBreakProps", "type TimelineItemsProps", "type TimelineProps", + "type TitleProps", "ToggleSwitch", "type ToggleSwitchProps", "Token", diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 9578768eee8..0e7382f7a91 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -208,7 +208,7 @@ export {Stack} from './Stack' export type {StackProps, StackItemProps} from './Stack' export {PageHeader} from './PageHeader' -export type {PageHeaderProps} from './PageHeader' +export type {PageHeaderProps, ParentLinkProps, TitleProps} from './PageHeader' export {default as sx, merge} from './sx' export type {BetterCssProperties, BetterSystemStyleObject, SxProp} from './sx' diff --git a/packages/react/src/utils/modern-polymorphic.ts b/packages/react/src/utils/modern-polymorphic.ts new file mode 100644 index 00000000000..5f83844ac10 --- /dev/null +++ b/packages/react/src/utils/modern-polymorphic.ts @@ -0,0 +1,35 @@ +// Mostly taken from https://github.com/total-typescript/react-typescript-tutorial/blob/main/src/08-advanced-patterns/72-as-prop-with-forward-ref.solution.tsx + +import {forwardRef} from 'react' +import type {ComponentPropsWithRef, ElementType} from 'react' + +/** + * Distributive Omit utility type that works correctly with union types + */ +type DistributiveOmit = T extends unknown ? Omit : never + +/** + * Fixed version of forwardRef that provides better type inference for polymorphic components + */ +// TODO: figure out how to change this type so we can set displayName +// like this: `ComponentName.displayName = 'DisplayName' instead of using workarounds +type FixedForwardRef = = Record>( + render: (props: P, ref: React.Ref) => React.ReactNode, +) => (props: P & React.RefAttributes) => React.ReactNode + +/** + * Cast forwardRef to the fixed version with better type inference + */ +export const fixedForwardRef = forwardRef as FixedForwardRef + +/** + * Simplified polymorphic props type that handles the common pattern of + * `DistributiveOmit, 'as'>` + */ +export type PolymorphicProps< + TAs extends ElementType, + TDefaultElement extends ElementType = 'div', + Props extends Record = Record, +> = DistributiveOmit & Props, 'as'> & { + as?: TAs +} diff --git a/packages/styled-react/src/components/PageHeader.tsx b/packages/styled-react/src/components/PageHeader.tsx new file mode 100644 index 00000000000..b119b490c44 --- /dev/null +++ b/packages/styled-react/src/components/PageHeader.tsx @@ -0,0 +1,54 @@ +import React, {type PropsWithChildren} from 'react' +import type { + PageHeaderProps as PrimerPageHeaderProps, + ParentLinkProps as PrimerParentLinkProps, + TitleProps as PrimerTitleProps, +} from '@primer/react' +import styled from 'styled-components' +import {PageHeader as PrimerPageHeader, Box} from '@primer/react' +import {sx, type SxProp} from '../sx' +import type {AriaRole} from '../types/AriaRole' + +type PageHeaderProps = PropsWithChildren< + // Needed to remove `role` and manually re-add because it's not exported from `@primer/react`. + // This was causing a type inference error on the `Object.assign` export at the bottom. + Omit & {role?: AriaRole} +> & + SxProp + +type PageHeaderTitleProps = PropsWithChildren & SxProp + +// Using Box crashes Vite for some reason? +const PageHeaderTitle = styled(PrimerPageHeader.Title)` + ${sx} +` + +type PageHeaderParentLinkProps = PropsWithChildren & SxProp + +const PageHeaderParentLink = React.forwardRef((props, ref) => { + return +}) + +// weird typecast to get around mysterious TS error +const PageHeader = Object.assign(PrimerPageHeader as unknown as React.FC, { + // Wrapped components that need sx support added back in + Title: PageHeaderTitle, + ParentLink: PageHeaderParentLink, + + // Re-exporting others directly + ContextArea: PrimerPageHeader.ContextArea, + ContextBar: PrimerPageHeader.ContextBar, + TitleArea: PrimerPageHeader.TitleArea, + ContextAreaActions: PrimerPageHeader.ContextAreaActions, + LeadingAction: PrimerPageHeader.LeadingAction, + Breadcrumbs: PrimerPageHeader.Breadcrumbs, + LeadingVisual: PrimerPageHeader.LeadingVisual, + TrailingVisual: PrimerPageHeader.TrailingVisual, + TrailingAction: PrimerPageHeader.TrailingAction, + Actions: PrimerPageHeader.Actions, + Description: PrimerPageHeader.Description, + Navigation: PrimerPageHeader.Navigation, +}) + +export {PageHeader} +export type {PageHeaderProps} diff --git a/packages/styled-react/src/index.tsx b/packages/styled-react/src/index.tsx index 66763c5389a..25c246c354a 100644 --- a/packages/styled-react/src/index.tsx +++ b/packages/styled-react/src/index.tsx @@ -28,6 +28,7 @@ import type { SpaceProps, TypographyProps, } from 'styled-system' +import {PageHeader} from './components/PageHeader' type StyledProps = SxProp & SpaceProps & @@ -90,7 +91,7 @@ const ToggleSwitch = forwardRef(function T return }) -export {SegmentedControl, StateLabel, SubNav, ToggleSwitch} +export {PageHeader, SegmentedControl, StateLabel, SubNav, ToggleSwitch} export { ActionList, @@ -115,7 +116,6 @@ export { LinkButton, NavList, Overlay, - PageHeader, PageLayout, ProgressBar, RadioGroup, diff --git a/packages/styled-react/src/sx.ts b/packages/styled-react/src/sx.ts new file mode 100644 index 00000000000..e3ff0277f10 --- /dev/null +++ b/packages/styled-react/src/sx.ts @@ -0,0 +1,8 @@ +import css from '@styled-system/css' +import type {SxProp} from '@primer/react' + +export const sx = (props: SxProp) => { + return css(props.sx) +} + +export type {SxProp} diff --git a/packages/styled-react/src/types/AriaRole.ts b/packages/styled-react/src/types/AriaRole.ts new file mode 100644 index 00000000000..ca8f062eb54 --- /dev/null +++ b/packages/styled-react/src/types/AriaRole.ts @@ -0,0 +1,72 @@ +// copied over from packages/react/src/utils/types/AriaRole.ts +// ref: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques +export type AriaRole = + | 'alert' + | 'alertdialog' + | 'application' + | 'article' + | 'banner' + | 'button' + | 'cell' + | 'checkbox' + | 'columnheader' + | 'combobox' + | 'complementary' + | 'contentinfo' + | 'definition' + | 'dialog' + | 'directory' + | 'document' + | 'feed' + | 'figure' + | 'form' + | 'grid' + | 'gridcell' + | 'group' + | 'heading' + | 'img' + | 'link' + | 'list' + | 'listbox' + | 'listitem' + | 'log' + | 'main' + | 'marquee' + | 'math' + | 'menu' + | 'menubar' + | 'menuitem' + | 'menuitemcheckbox' + | 'menuitemradio' + | 'navigation' + | 'none' + | 'note' + | 'option' + | 'presentation' + | 'progressbar' + | 'radio' + | 'radiogroup' + | 'region' + | 'row' + | 'rowgroup' + | 'rowheader' + | 'scrollbar' + | 'search' + | 'searchbox' + | 'separator' + | 'slider' + | 'spinbutton' + | 'status' + | 'switch' + | 'tab' + | 'table' + | 'tablist' + | 'tabpanel' + | 'term' + | 'textbox' + | 'timer' + | 'toolbar' + | 'tooltip' + | 'tree' + | 'treegrid' + | 'treeitem'