Remove Box usage and sx prop from PageHeader

.changeset/plenty-friends-flash.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': major
+---
+
+Removes sx prop from PageHeader and subcomponents

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<HTMLDivElement, React.PropsWithChildren<PageHeaderProps>>(
- ({children, className, sx = defaultSxProp, as = 'div', 'aria-label': ariaLabel, role, hasBorder}, forwardedRef) => {
- const rootRef = useProvidedRefOrCreate<HTMLDivElement>(forwardedRef as React.RefObject<HTMLDivElement>)
-
- 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<TAs extends React.ElementType = 'div'>(
+ props: PolymorphicProps<TAs, 'div'> & PageHeaderProps,
+ forwardedRef: React.ForwardedRef<unknown>,
+) {
+ const {
+ children,
+ className,
+ sx = defaultSxProp,
+ as = 'div',
+ 'aria-label': ariaLabel,
+ role,
+ hasBorder,
+ ...restProps
+ } = props
+ const rootRef = useProvidedRefOrCreate<HTMLDivElement>(forwardedRef as React.RefObject<HTMLDivElement>)
+
+ 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 (
- <BoxWithFallback
- as={as}
- ref={rootRef}
- className={clsx(classes.PageHeader, className)}
- data-has-border={hasBorder ? 'true' : undefined}
- sx={sx}
- aria-label={ariaLabel}
- role={role}
- >
- {children}
- </BoxWithFallback>
- )
- },
-) as PolymorphicForwardRefComponent<'div', PageHeaderProps>
+ return (
+ <BoxWithFallback
+ as={as}
+ ref={rootRef}
+ className={clsx(classes.PageHeader, className)}
+ data-has-border={hasBorder ? 'true' : undefined}
+ sx={sx}
+ aria-label={ariaLabel}
+ role={role}
+ {...restProps}
+ >
+ {children}
+ </BoxWithFallback>
+ )
+}
+
+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<React.PropsWithChildren<ChildrenPropTypes>> = ({
  </BoxWithFallback>
  )
 }
-type LinkProps = Pick<
+type LinkProps<As extends React.ElementType = 'a'> = Pick<
  React.AnchorHTMLAttributes<HTMLAnchorElement> & BaseLinkProps,
  'download' | 'href' | 'hrefLang' | 'media' | 'ping' | 'rel' | 'target' | 'type' | 'referrerPolicy' | 'as'
 > & {
  'aria-label'?: React.AriaAttributes['aria-label']
+ as?: As
 }
 export type ParentLinkProps = React.PropsWithChildren<ChildrenPropTypes & LinkProps>
 
-// PageHeader.ParentLink : Only visible on narrow viewports by default to let users navigate up in the hierarchy.
-const ParentLink = React.forwardRef<HTMLAnchorElement, ParentLinkProps>(
- (
- {
- children,
- className,
- sx: sxProp = defaultSxProp,
- href,
- 'aria-label': ariaLabel,
- as = 'a',
- hidden = hiddenOnRegularAndWide,
- },
- ref,
- ) => {
- return (
- <>
- <Link
- ref={ref}
- as={as}
- aria-label={ariaLabel}
- muted
- className={clsx(classes.ParentLink, className)}
- sx={sxProp}
- {...getHiddenDataAttributes(hidden)}
- href={href}
- >
- <ArrowLeftIcon />
- <div>{children}</div>
- </Link>
- </>
- )
- },
-) as PolymorphicForwardRefComponent<'a', ParentLinkProps>
-ParentLink.displayName = 'ParentLink'
+function UnwrappedParentLink<TAs extends React.ElementType = 'a'>(
+ props: React.PropsWithChildren<ChildrenPropTypes & LinkProps & PolymorphicProps<TAs, 'a'>>,
+ forwardedRef: React.ForwardedRef<unknown>,
+) {
+ const {children, className, 'aria-label': ariaLabel, as = 'a', hidden = hiddenOnRegularAndWide, ...restProps} = props
+
+ return (
+ <>
+ <Link
+ ref={forwardedRef as React.RefObject<HTMLAnchorElement>}
+ as={as}
+ aria-label={ariaLabel}
+ muted
+ className={clsx(classes.ParentLink, className)}
+ {...getHiddenDataAttributes(hidden)}
+ {...restProps}
+ >
+ <ArrowLeftIcon />
+ <div>{children}</div>
+ </Link>
+ </>
+ )
+}
+
+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<HTMLDivElement, React.PropsWithChildren<TitleAreaProps>>(
- ({children, className, sx: sxProp = defaultSxProp, hidden = false, variant = 'medium'}, forwardedRef) => {
- const titleAreaRef = useProvidedRefOrCreate<HTMLDivElement>(forwardedRef as React.RefObject<HTMLDivElement>)
- const currentVariant = useResponsiveValue(variant, 'medium')
- return (
- <BoxWithFallback
- className={clsx(classes.TitleArea, className)}
- ref={titleAreaRef}
- data-component="TitleArea"
- data-size-variant={currentVariant}
- sx={sxProp}
- {...getHiddenDataAttributes(hidden)}
- >
- {children}
- </BoxWithFallback>
- )
- },
-) as PolymorphicForwardRefComponent<'div', TitleAreaProps>
-TitleArea.displayName = 'TitleArea'
+function UnwrappedTitleArea<TAs extends React.ElementType = 'div'>(
+ props: React.PropsWithChildren<TitleAreaProps & PolymorphicProps<TAs, 'div'>>,
+ forwardedRef: React.ForwardedRef<unknown>,
+) {
+ const {children, className, sx: sxProp = defaultSxProp, hidden = false, variant = 'medium', ...restProps} = props
+ const titleAreaRef = useProvidedRefOrCreate<HTMLDivElement>(forwardedRef as React.RefObject<HTMLDivElement>)
+ const currentVariant = useResponsiveValue(variant, 'medium')
+ return (
+ <BoxWithFallback
+ className={clsx(classes.TitleArea, className)}
+ ref={titleAreaRef}
+ data-component="TitleArea"
+ data-size-variant={currentVariant}
+ sx={sxProp}
+ {...getHiddenDataAttributes(hidden)}
+ {...restProps}
+ >
+ {children}
+ </BoxWithFallback>
+ )
+}
+
+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<React.PropsWithChildren<TitleProps>> = ({
  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'

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",

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'

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, TOmitted extends PropertyKey> = T extends unknown ? Omit<T, TOmitted> : 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 = <T, P extends Record<string, unknown> = Record<string, unknown>>(
+ render: (props: P, ref: React.Ref<T>) => React.ReactNode,
+) => (props: P & React.RefAttributes<T>) => 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<ComponentPropsWithRef<ElementType extends As ? DefaultElement : As>, 'as'>`
+ */
+export type PolymorphicProps<
+ TAs extends ElementType,
+ TDefaultElement extends ElementType = 'div',
+ Props extends Record<string, unknown> = Record<string, unknown>,
+> = DistributiveOmit<ComponentPropsWithRef<ElementType extends TAs ? TDefaultElement : TAs> & Props, 'as'> & {
+ as?: TAs
+}