From d95b2f9f718feec4b18708f57dda2378b752b808 Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Wed, 10 Sep 2025 09:14:45 -0400 Subject: [PATCH 01/47] rm Box and sx prop from components, add to styled-react --- .../react/src/Checkbox/Checkbox.docs.json | 12 +- packages/react/src/Checkbox/Checkbox.tsx | 29 +-- packages/react/src/Dialog/Dialog.module.css | 12 ++ packages/react/src/Dialog/Dialog.tsx | 56 +++--- packages/react/src/Heading/Heading.docs.json | 5 - packages/react/src/Heading/Heading.tsx | 42 ++--- .../src/Heading/__tests__/Heading.test.tsx | 130 ------------- packages/react/src/Link/Link.docs.json | 5 - packages/react/src/Link/Link.tsx | 49 ++--- .../react/src/Link/__tests__/Link.test.tsx | 11 -- packages/react/src/NavList/NavList.docs.json | 42 +---- packages/react/src/NavList/NavList.module.css | 8 + packages/react/src/NavList/NavList.tsx | 100 ++-------- packages/react/src/PageHeader/PageHeader.tsx | 15 +- .../src/PageLayout/PageLayout.module.css | 20 ++ packages/react/src/PageLayout/PageLayout.tsx | 141 ++++++-------- .../src/UnderlineNav/UnderlineNav.docs.json | 10 - .../src/UnderlineNav/UnderlineNav.module.css | 77 ++++++++ .../src/UnderlineNav/UnderlineNav.test.tsx | 12 +- .../react/src/UnderlineNav/UnderlineNav.tsx | 83 ++++----- .../src/UnderlineNav/UnderlineNavItem.tsx | 25 +-- packages/react/src/index.ts | 3 +- .../react/src/utils/modern-polymorphic.ts | 30 +++ .../__tests__/primer-react.browser.test.tsx | 172 +++++++++++++++++- .../styled-react/src/components/NavList.tsx | 73 ++++++++ .../src/components/PageHeader.tsx | 52 ++++++ .../src/components/PageLayout.tsx | 58 ++++++ .../src/components/UnderlineNav.tsx | 27 +++ packages/styled-react/src/index.tsx | 39 +++- packages/styled-react/src/sx.ts | 8 + 30 files changed, 750 insertions(+), 596 deletions(-) create mode 100644 packages/react/src/NavList/NavList.module.css create mode 100644 packages/react/src/UnderlineNav/UnderlineNav.module.css create mode 100644 packages/react/src/utils/modern-polymorphic.ts create mode 100644 packages/styled-react/src/components/NavList.tsx create mode 100644 packages/styled-react/src/components/PageHeader.tsx create mode 100644 packages/styled-react/src/components/PageLayout.tsx create mode 100644 packages/styled-react/src/components/UnderlineNav.tsx create mode 100644 packages/styled-react/src/sx.ts diff --git a/packages/react/src/Checkbox/Checkbox.docs.json b/packages/react/src/Checkbox/Checkbox.docs.json index 8fb26dce144..7b66a34ec71 100644 --- a/packages/react/src/Checkbox/Checkbox.docs.json +++ b/packages/react/src/Checkbox/Checkbox.docs.json @@ -67,17 +67,7 @@ { "name": "ref", "type": "React.RefObject" - }, - { - "name": "as", - "type": "React.ElementType", - "defaultValue": "\"input\"" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "subcomponents": [] -} \ No newline at end of file +} diff --git a/packages/react/src/Checkbox/Checkbox.tsx b/packages/react/src/Checkbox/Checkbox.tsx index 5a6ce5adc43..6b05d5203ac 100644 --- a/packages/react/src/Checkbox/Checkbox.tsx +++ b/packages/react/src/Checkbox/Checkbox.tsx @@ -1,13 +1,11 @@ import {clsx} from 'clsx' import {useProvidedRefOrCreate} from '../hooks' import React, {useContext, useEffect, type ChangeEventHandler, type InputHTMLAttributes, type ReactElement} from 'react' -import {type SxProp} from '../sx' import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' import type {FormValidationStatus} from '../utils/types/FormValidationStatus' import {CheckboxGroupContext} from '../CheckboxGroup/CheckboxGroupContext' import classes from './Checkbox.module.css' import sharedClasses from './shared.module.css' -import Box from '../Box' export type CheckboxProps = { /** @@ -35,27 +33,14 @@ export type CheckboxProps = { * Used during form submission and to identify which checkbox inputs are selected */ value?: string -} & Exclude, 'value'> & - SxProp +} & Exclude, 'value'> /** * An accessible, native checkbox component */ const Checkbox = React.forwardRef( ( - { - checked, - className, - defaultChecked, - indeterminate, - disabled, - onChange, - sx: sxProp, - required, - validationStatus, - value, - ...rest - }, + {checked, className, defaultChecked, indeterminate, disabled, onChange, required, validationStatus, value, ...rest}, ref, ): ReactElement => { const checkboxRef = useProvidedRefOrCreate(ref as React.RefObject) @@ -99,16 +84,6 @@ const Checkbox = React.forwardRef( } }) - if (sxProp) { - return ( - - ) - } return }, ) diff --git a/packages/react/src/Dialog/Dialog.module.css b/packages/react/src/Dialog/Dialog.module.css index fb773a6b1b4..d750854c158 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -270,6 +270,18 @@ Add a border between the body and footer if: flex-shrink: 0; } +.HeaderInner { + display: flex; +} + +.HeaderContent { + display: flex; + padding-inline: var(--base-size-8); + padding-block: var(--base-size-6); + flex-direction: column; + flex-grow: 1; +} + .Title { margin: 0; /* override default margin */ font-size: var(--text-body-size-medium); diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 14f6151ede9..54a56ac4e69 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -1,10 +1,8 @@ import React, {useCallback, useEffect, useRef, useState, type SyntheticEvent} from 'react' import type {ButtonProps} from '../Button' import {Button, IconButton} from '../Button' -import Box from '../Box' import {useOnEscapePress, useProvidedRefOrCreate} from '../hooks' import {useFocusTrap} from '../hooks/useFocusTrap' -import type {SxProp} from '../sx' import {XIcon} from '@primer/octicons-react' import {useFocusZone} from '../hooks/useFocusZone' import {FocusKeys} from '@primer/behaviors' @@ -17,7 +15,6 @@ import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../uti import classes from './Dialog.module.css' import {clsx} from 'clsx' -import {BoxWithFallback} from '../internal/components/BoxWithFallback' /* Dialog Version 2 */ @@ -52,7 +49,7 @@ export type DialogButtonProps = Omit & { /** * Props to customize the rendering of the Dialog. */ -export interface DialogProps extends SxProp { +export interface DialogProps { /** * Title of the Dialog. Also serves as the aria-label for this Dialog. */ @@ -198,13 +195,13 @@ const DefaultHeader: React.FC> = ({ }, [onClose]) return ( - - +
+
{title ?? 'Dialog'} {subtitle && {subtitle}} - +
- +
) } @@ -245,7 +242,6 @@ const _Dialog = React.forwardRef - - {header} @@ -339,50 +332,51 @@ const _Dialog = React.forwardRef {footer} - - + + ) }) _Dialog.displayName = 'Dialog' -type StyledHeaderProps = React.ComponentProps<'div'> & SxProp +type StyledHeaderProps = React.ComponentProps<'div'> -const Header = React.forwardRef(function Header({className, ...rest}, forwardRef) { - return +const Header = React.forwardRef(function Header({className, ...rest}, forwardRef) { + return
}) Header.displayName = 'Dialog.Header' -type StyledTitleProps = React.ComponentProps<'h1'> & SxProp +type StyledTitleProps = React.ComponentProps<'h1'> -const Title = React.forwardRef(function Title({className, ...rest}, forwardRef) { - return +// TODO: get rid of typecasting `forwardRef` +const Title = React.forwardRef(function Title({className, ...rest}, forwardRef) { + return

}) Title.displayName = 'Dialog.Title' -type StyledSubtitleProps = React.ComponentProps<'h2'> & SxProp +type StyledSubtitleProps = React.ComponentProps<'h2'> -const Subtitle = React.forwardRef(function Subtitle( +const Subtitle = React.forwardRef(function Subtitle( {className, ...rest}, forwardRef, ) { - return + return

}) Subtitle.displayName = 'Dialog.Subtitle' -type StyledBodyProps = React.ComponentProps<'div'> & SxProp +type StyledBodyProps = React.ComponentProps<'div'> -const Body = React.forwardRef(function Body({className, ...rest}, forwardRef) { - return +const Body = React.forwardRef(function Body({className, ...rest}, forwardRef) { + return
}) as PolymorphicForwardRefComponent<'div', StyledBodyProps> Body.displayName = 'Dialog.Body' -type StyledFooterProps = React.ComponentProps<'div'> & SxProp +type StyledFooterProps = React.ComponentProps<'div'> -const Footer = React.forwardRef(function Footer({className, ...rest}, forwardRef) { - return +const Footer = React.forwardRef(function Footer({className, ...rest}, forwardRef) { + return
}) Footer.displayName = 'Dialog.Footer' diff --git a/packages/react/src/Heading/Heading.docs.json b/packages/react/src/Heading/Heading.docs.json index eda0b578973..85f20e94203 100644 --- a/packages/react/src/Heading/Heading.docs.json +++ b/packages/react/src/Heading/Heading.docs.json @@ -6,11 +6,6 @@ "stories": [], "importPath": "@primer/react", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "as", "type": "React.ElementType", diff --git a/packages/react/src/Heading/Heading.tsx b/packages/react/src/Heading/Heading.tsx index 3064d1b82f3..dafee489847 100644 --- a/packages/react/src/Heading/Heading.tsx +++ b/packages/react/src/Heading/Heading.tsx @@ -1,18 +1,20 @@ import {clsx} from 'clsx' -import React, {forwardRef, useEffect} from 'react' +import React, {useEffect} from 'react' import {useRefObjectAsForwardedRef} from '../hooks' -import type {SxProp} from '../sx' import type {ComponentProps} from '../utils/types' -import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import classes from './Heading.module.css' -import Box from '../Box' +import {type PolymorphicProps, fixedForwardRef} from '../utils/modern-polymorphic' -type StyledHeadingProps = { - as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' - variant?: 'large' | 'medium' | 'small' -} & SxProp +type HeadingElement = 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' -const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}, forwardedRef) => { +const UnwrappedHeading = ( + props: { + as?: As + variant?: 'large' | 'medium' | 'small' + } & PolymorphicProps, + forwardedRef: React.ForwardedRef, +) => { + const {as: Component = 'h2', className, variant, ...restProps} = props const innerRef = React.useRef(null) useRefObjectAsForwardedRef(forwardedRef, innerRef) @@ -33,22 +35,14 @@ const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props} }, [innerRef]) } - if (props.sx) { - return ( - - ) - } - return -}) as PolymorphicForwardRefComponent<'h2', StyledHeadingProps> + return +} + +const Heading = fixedForwardRef(UnwrappedHeading) -Heading.displayName = 'Heading' +// We can't assign displayName to the result of fixedForwardRef directly, +// so we assign it to the component and cast it to the expected type +Object.assign(Heading, {displayName: 'Heading'}) export type HeadingProps = ComponentProps export default Heading diff --git a/packages/react/src/Heading/__tests__/Heading.test.tsx b/packages/react/src/Heading/__tests__/Heading.test.tsx index 7b9836e3af6..573bc2405e9 100644 --- a/packages/react/src/Heading/__tests__/Heading.test.tsx +++ b/packages/react/src/Heading/__tests__/Heading.test.tsx @@ -1,30 +1,6 @@ import {describe, expect, it, vi} from 'vitest' import {Heading} from '../..' import {render, screen} from '@testing-library/react' -import ThemeProvider from '../../ThemeProvider' - -const theme = { - breakpoints: ['400px', '640px', '960px', '1280px'], - colors: { - green: ['#010', '#020', '#030', '#040', '#050', '#060'], - }, - fontSizes: ['12px', '14px', '16px', '20px', '24px', '32px', '40px', '48px'], - fonts: { - normal: 'Helvetica,sans-serif', - mono: 'Consolas,monospace', - }, - lineHeights: { - normal: 1.5, - condensed: 1.25, - condensedUltra: 1, - }, - fontWeights: { - light: '300', - normal: '400', - semibold: '500', - bold: '600', - }, -} describe('Heading', () => { it('should support `className` on the outermost element', () => { @@ -38,89 +14,6 @@ describe('Heading', () => { expect(heading.tagName).toBe('H2') }) - it('respects fontWeight', () => { - const {container} = render( - - - , - ) - const heading = container.firstChild as HTMLElement - expect(heading).toHaveStyle(`font-weight: ${theme.fontWeights.bold}`) - - const {container: container2} = render( - - - , - ) - const heading2 = container2.firstChild as HTMLElement - expect(heading2).toHaveStyle(`font-weight: ${theme.fontWeights.normal}`) - - const {container: container3} = render( - - - , - ) - const heading3 = container3.firstChild as HTMLElement - expect(heading3).toHaveStyle(`font-weight: ${theme.fontWeights.semibold}`) - - const {container: container4} = render( - - - , - ) - const heading4 = container4.firstChild as HTMLElement - expect(heading4).toHaveStyle(`font-weight: ${theme.fontWeights.light}`) - }) - - it('respects lineHeight', () => { - const {container} = render( - - - , - ) - const heading = container.firstChild as HTMLElement - ///These sx tests should go away right? - expect(heading).toHaveStyle(`line-height: 48px`) - - const {container: container2} = render( - - - , - ) - const heading2 = container2.firstChild as HTMLElement - expect(heading2).toHaveStyle(`line-height: 40px`) - - const {container: container3} = render( - - - , - ) - const heading3 = container3.firstChild as HTMLElement - expect(heading3).toHaveStyle(`line-height: 32px`) - }) - - it('respects fontFamily="mono"', () => { - const {container} = render( - - - , - ) - const heading = container.firstChild as HTMLElement - expect(heading).toHaveStyle(`font-family: ${theme.fonts.mono}`) - }) - - it('renders fontSize', () => { - for (const fontSize of theme.fontSizes) { - const {container} = render( - - - , - ) - const heading = container.firstChild as HTMLElement - expect(heading).toHaveStyle(`font-size: ${fontSize}`) - } - }) - it('logs a warning when trying to render invalid "as" prop', () => { const consoleSpy = vi.spyOn(globalThis.console, 'warn').mockImplementation(() => {}) @@ -130,15 +23,6 @@ describe('Heading', () => { consoleSpy.mockRestore() }) - it('respects the "fontStyle" prop', () => { - const {container} = render( - - - , - ) - const heading = container.firstChild as HTMLElement - expect(heading).toHaveStyle('font-style: italic') - }) // How can we test for generated class names? it.skip('should only include css modules class', () => { @@ -148,18 +32,4 @@ describe('Heading', () => { // for this component expect(screen.getByText('test')).not.toHaveClass(/^Heading__StyledHeading/) }) - - it('should support overrides with sx if provided', () => { - render( - - test - , - ) - - expect(screen.getByText('test')).toHaveStyle('font-weight: 900') - }) }) diff --git a/packages/react/src/Link/Link.docs.json b/packages/react/src/Link/Link.docs.json index a340feb72bc..8887776364e 100644 --- a/packages/react/src/Link/Link.docs.json +++ b/packages/react/src/Link/Link.docs.json @@ -58,11 +58,6 @@ "name": "as", "type": "React.ElementType", "defaultValue": "\"a\"" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "subcomponents": [] diff --git a/packages/react/src/Link/Link.tsx b/packages/react/src/Link/Link.tsx index bba00e98cb6..5cde5ffaf09 100644 --- a/packages/react/src/Link/Link.tsx +++ b/packages/react/src/Link/Link.tsx @@ -1,11 +1,10 @@ import {clsx} from 'clsx' -import React, {forwardRef, useEffect} from 'react' +import React, {useEffect} from 'react' +import type {ElementType, ForwardedRef} from 'react' import {useRefObjectAsForwardedRef} from '../hooks' -import type {SxProp} from '../sx' import classes from './Link.module.css' -import Box from '../Box' import type {ComponentProps} from '../utils/types' -import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {type PolymorphicProps, fixedForwardRef} from '../utils/modern-polymorphic' type StyledLinkProps = { /** @deprecated use CSS modules to style hover color */ @@ -15,11 +14,18 @@ type StyledLinkProps = { underline?: boolean // Link inside a text block inline?: boolean -} & SxProp +} -const Link = forwardRef(({as: Component = 'a', className, inline, underline, hoverColor, ...props}, forwardedRef) => { - const innerRef = React.useRef(null) - useRefObjectAsForwardedRef(forwardedRef, innerRef) +export const UnwrappedLink = ( + props: { + as?: As + } & StyledLinkProps & + PolymorphicProps, + ref: ForwardedRef, +) => { + const {as: Component = 'a', className, inline, underline, hoverColor, ...restProps} = props + const innerRef = React.useRef(null) + useRefObjectAsForwardedRef(ref, innerRef) if (__DEV__) { /** @@ -46,37 +52,22 @@ const Link = forwardRef(({as: Component = 'a', className, inline, underline, hov }, [innerRef]) } - if (props.sx) { - return ( - - ) - } - return ( ) -}) as PolymorphicForwardRefComponent<'a', StyledLinkProps> +} + +const LinkComponent = fixedForwardRef(UnwrappedLink) -Link.displayName = 'Link' +const Link = Object.assign(LinkComponent, {displayName: 'Link'}) export type LinkProps = ComponentProps export default Link diff --git a/packages/react/src/Link/__tests__/Link.test.tsx b/packages/react/src/Link/__tests__/Link.test.tsx index e43fe5737c4..142ab485496 100644 --- a/packages/react/src/Link/__tests__/Link.test.tsx +++ b/packages/react/src/Link/__tests__/Link.test.tsx @@ -18,11 +18,6 @@ describe('Link', () => { expect(container.firstChild).toHaveStyle('--fgColor-accent: #0969da') }) - it('respects the "sx" prop', () => { - const {container} = render() - expect(container.firstChild).toHaveStyle('font-style: italic') - }) - it('applies button styles when rendering a button element', () => { const {container} = render() expect((container.firstChild as Element).tagName).toBe('BUTTON') @@ -33,12 +28,6 @@ describe('Link', () => { expect(container.firstChild).toHaveAttribute('data-muted', 'true') }) - it('respects the "sx" prop when "muted" prop is also passed', () => { - const {container} = render() - expect(container.firstChild).toHaveAttribute('data-muted', 'true') - expect(container.firstChild).toHaveStyle('color: rgb(89, 99, 110)') - }) - it('logs a warning when trying to render invalid "as" prop', () => { const consoleSpy = vi.spyOn(globalThis.console, 'error').mockImplementation(() => {}) diff --git a/packages/react/src/NavList/NavList.docs.json b/packages/react/src/NavList/NavList.docs.json index e3315ab91ab..d1800f44558 100644 --- a/packages/react/src/NavList/NavList.docs.json +++ b/packages/react/src/NavList/NavList.docs.json @@ -26,11 +26,6 @@ "name": "as", "type": "React.ElementType", "defaultValue": "\"nav\"" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "subcomponents": [ @@ -68,11 +63,6 @@ "name": "as", "type": "React.ElementType", "defaultValue": "\"a\"" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "passthrough": { @@ -83,11 +73,6 @@ { "name": "NavList.LeadingVisual", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -97,11 +82,6 @@ { "name": "NavList.TrailingVisual", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -111,11 +91,6 @@ { "name": "NavList.SubNav", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -137,11 +112,6 @@ "type": "string", "description": "The text that gets rendered as the group's heading. Alternatively, you can pass the `NavList.GroupHeading` component as a child of `NavList.Group`.\nIf both `title` and `NavList.GroupHeading` are passed, `NavList.GroupHeading` will be rendered as the heading." }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -172,11 +142,6 @@ "description": "", "defaultValue": "" }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -186,11 +151,6 @@ { "name": "NavList.Divider", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -264,4 +224,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/packages/react/src/NavList/NavList.module.css b/packages/react/src/NavList/NavList.module.css new file mode 100644 index 00000000000..9908361e112 --- /dev/null +++ b/packages/react/src/NavList/NavList.module.css @@ -0,0 +1,8 @@ +.GroupHeading > a { + color: var(--fgColor-default); + text-decoration: inherit; +} + +.GroupHeading > a:hover { + text-decoration: underline; +} diff --git a/packages/react/src/NavList/NavList.tsx b/packages/react/src/NavList/NavList.tsx index 360558fded8..fa75e61834e 100644 --- a/packages/react/src/NavList/NavList.tsx +++ b/packages/react/src/NavList/NavList.tsx @@ -1,6 +1,7 @@ import {ChevronDownIcon, PlusIcon, type Icon} from '@primer/octicons-react' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import React, {isValidElement} from 'react' +import {clsx} from 'clsx' import type { ActionListTrailingActionProps, ActionListDividerProps, @@ -11,27 +12,22 @@ import type { import {ActionList} from '../ActionList' import {SubItem} from '../ActionList/Item' import {ActionListContainerContext} from '../ActionList/ActionListContainerContext' -import Box from '../Box' -import type {SxProp} from '../sx' -import {merge} from '../sx' -import {defaultSxProp} from '../utils/defaultSxProp' import {useId} from '../hooks/useId' import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' import classes from '../ActionList/ActionList.module.css' +import navListClasses from './NavList.module.css' import {flushSync} from 'react-dom' -import {BoxWithFallback} from '../internal/components/BoxWithFallback' // ---------------------------------------------------------------------------- // NavList export type NavListProps = { children: React.ReactNode -} & SxProp & - React.ComponentProps<'nav'> +} & React.ComponentProps<'nav'> const Root = React.forwardRef(({children, ...props}, ref) => { return ( - + ) }) @@ -54,10 +50,10 @@ export type NavListItemProps = { href?: string 'aria-current'?: 'page' | 'step' | 'location' | 'date' | 'time' | 'true' | 'false' | boolean inactiveText?: string -} & SxProp +} const Item = React.forwardRef( - ({'aria-current': ariaCurrent, children, defaultOpen, sx: sxProp = defaultSxProp, ...props}, ref) => { + ({'aria-current': ariaCurrent, children, defaultOpen, ...props}, ref) => { const {depth} = React.useContext(SubNavContext) // Get SubNav from children @@ -79,7 +75,6 @@ const Item = React.forwardRef( subNav={subNav} depth={depth} defaultOpen={defaultOpen} - sx={sxProp} style={{'--subitem-depth': depth} as React.CSSProperties} > {childrenWithoutSubNavOrTrailingAction} @@ -112,7 +107,7 @@ type ItemWithSubNavProps = { depth: number defaultOpen?: boolean style: React.CSSProperties -} & SxProp +} const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: string; isOpen: boolean}>({ buttonId: '', @@ -120,14 +115,7 @@ const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: s isOpen: false, }) -function ItemWithSubNav({ - children, - subNav, - depth: _depth, - defaultOpen, - style, - sx: sxProp = defaultSxProp, -}: ItemWithSubNavProps) { +function ItemWithSubNav({children, subNav, depth: _depth, defaultOpen, style}: ItemWithSubNavProps) { const buttonId = useId() const subNavId = useId() const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? false) @@ -147,28 +135,6 @@ function ItemWithSubNav({ } }, [subNav, buttonId]) - if (sxProp !== defaultSxProp) { - return ( - - setIsOpen(open => !open)} - style={style} - sx={sxProp} - > - {children} - {/* What happens if the user provides a TrailingVisual? */} - - - - {React.cloneElement(subNav as React.ReactElement, {ref: subNavRef})} - - - ) - } return ( setIsOpen(open => !open)} + onSelect={() => setIsOpen((open: boolean) => !open)} style={style} > {children} @@ -195,12 +161,12 @@ function ItemWithSubNav({ export type NavListSubNavProps = { children: React.ReactNode -} & SxProp +} const SubNavContext = React.createContext<{depth: number}>({depth: 0}) // NOTE: SubNav must be a direct child of an Item -const SubNav = React.forwardRef(({children, sx: sxProp = defaultSxProp}: NavListSubNavProps, forwardedRef) => { +const SubNav = React.forwardRef(({children}, forwardedRef) => { const {buttonId, subNavId} = React.useContext(ItemWithSubNavContext) const {depth} = React.useContext(SubNavContext) if (!buttonId || !subNavId) { @@ -215,22 +181,6 @@ const SubNav = React.forwardRef(({children, sx: sxProp = defaultSxProp}: NavList return null } - if (sxProp !== defaultSxProp) { - return ( - - - {children} - - - ) - } return (
    @@ -283,18 +233,9 @@ TrailingAction.displayName = 'NavList.TrailingAction' export type NavListGroupProps = React.HTMLAttributes & { children: React.ReactNode title?: string -} & SxProp +} -const defaultSx = {} -const Group: React.FC = ({title, children, sx: sxProp = defaultSx, ...props}) => { - if (sxProp !== defaultSx) { - return ( - - {title ? {title} : null} - {children} - - ) - } +const Group: React.FC = ({title, children, ...props}) => { return ( <> @@ -419,20 +360,11 @@ export type NavListGroupHeadingProps = ActionListGroupHeadingProps * This is an alternative to the `title` prop on `NavList.Group`. * It was primarily added to allow links in group headings. */ -const GroupHeading: React.FC = ({as = 'h3', sx: sxProp = defaultSxProp, ...rest}) => { +const GroupHeading: React.FC = ({as = 'h3', className, ...rest}) => { return ( ( - { - '> a {': { - color: 'var(--fgColor-default)', - textDecoration: 'inherit', - ':hover': {textDecoration: 'underline'}, - }, - }, - sxProp, - )} + className={clsx(navListClasses.GroupHeading, className)} data-component="NavList.GroupHeading" headingWrapElement="li" {...rest} diff --git a/packages/react/src/PageHeader/PageHeader.tsx b/packages/react/src/PageHeader/PageHeader.tsx index f78a6890b12..269fa36586c 100644 --- a/packages/react/src/PageHeader/PageHeader.tsx +++ b/packages/react/src/PageHeader/PageHeader.tsx @@ -146,18 +146,7 @@ export type ParentLinkProps = React.PropsWithChildren( - ( - { - children, - className, - sx: sxProp = defaultSxProp, - href, - 'aria-label': ariaLabel, - as = 'a', - hidden = hiddenOnRegularAndWide, - }, - ref, - ) => { + ({children, className, href, 'aria-label': ariaLabel, as = 'a', hidden = hiddenOnRegularAndWide}, ref) => { return ( <> ( aria-label={ariaLabel} muted className={clsx(classes.ParentLink, className)} - sx={sxProp} {...getHiddenDataAttributes(hidden)} href={href} > @@ -336,7 +324,6 @@ const Title: React.FC> = ({ data-hidden={hidden} as={as} style={style} - sx={sxProp} {...getHiddenDataAttributes(hidden)} > {children} diff --git a/packages/react/src/PageLayout/PageLayout.module.css b/packages/react/src/PageLayout/PageLayout.module.css index 53a33ce483d..453ccba6ddb 100644 --- a/packages/react/src/PageLayout/PageLayout.module.css +++ b/packages/react/src/PageLayout/PageLayout.module.css @@ -325,3 +325,23 @@ body[data-page-layout-dragging='true'] * { /* stylelint-disable-next-line primer/spacing */ padding: var(--spacing); } + +.DraggableHandle { + position: absolute; + inset: 0 -2px; + cursor: col-resize; + background-color: transparent; + transition-delay: 0.1s; +} + +.DraggableHandle:hover { + background-color: var(--bgColor-neutral-muted); +} + +.DraggableHandle[data-dragging='true'] { + background-color: var(--bgColor-accent-emphasis); +} + +.DraggableHandle[data-dragging='true']:hover { + background-color: var(--bgColor-accent-emphasis); +} diff --git a/packages/react/src/PageLayout/PageLayout.tsx b/packages/react/src/PageLayout/PageLayout.tsx index 1db19d71951..4fc12fe3805 100644 --- a/packages/react/src/PageLayout/PageLayout.tsx +++ b/packages/react/src/PageLayout/PageLayout.tsx @@ -1,18 +1,15 @@ import React, {useRef} from 'react' import {clsx} from 'clsx' -import Box from '../Box' import {useId} from '../hooks/useId' import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import type {ResponsiveValue} from '../hooks/useResponsiveValue' import {isResponsiveValue, useResponsiveValue} from '../hooks/useResponsiveValue' import {useSlots} from '../hooks/useSlots' -import type {SxProp} from '../sx' import {canUseDOM} from '../utils/environment' import {useOverflow} from '../hooks/useOverflow' import {warning} from '../utils/warning' import classes from './PageLayout.module.css' -import {BoxWithFallback} from '../internal/components/BoxWithFallback' const REGION_ORDER = { header: 0, @@ -56,7 +53,7 @@ export type PageLayoutProps = { _slotsConfig?: Record<'header' | 'footer', React.ElementType> className?: string style?: React.CSSProperties -} & SxProp +} // eslint-disable-next-line @typescript-eslint/no-unused-vars const containerWidths = { @@ -73,7 +70,6 @@ const Root: React.FC> = ({ rowGap = 'normal', columnGap = 'normal', children, - sx, className, style, _slotsConfig: slotsConfig, @@ -93,14 +89,13 @@ const Root: React.FC> = ({ return ( -
    @@ -108,7 +103,7 @@ const Root: React.FC> = ({
    {rest}
    {slots.footer}
    -     +
) } @@ -123,11 +118,10 @@ type DividerProps = { className?: string style?: React.CSSProperties position?: keyof typeof panePositions -} & SxProp +} const HorizontalDivider: React.FC> = ({ variant = 'none', - sx, className, position, style, @@ -136,8 +130,7 @@ const HorizontalDivider: React.FC> = ({ const responsiveVariant = useResponsiveValue(variant, 'none') return ( - { const [isDragging, setIsDragging] = React.useState(false) const [isKeyboardDrag, setIsKeyboardDrag] = React.useState(false) @@ -268,8 +260,7 @@ const VerticalDivider: React.FC {draggable ? ( // Drag handle - <> - { - if (event.button === 0) { - setIsDragging(true) - onDragStart?.() - } - }} - onKeyDown={event => { - if ( - event.key === 'ArrowLeft' || - event.key === 'ArrowRight' || - event.key === 'ArrowUp' || - event.key === 'ArrowDown' - ) { - setIsKeyboardDrag(true) - onDragStart?.() - } - }} - onDoubleClick={onDoubleClick} - /> - +
{ + if (event.button === 0) { + setIsDragging(true) + onDragStart?.() + } + }} + onKeyDown={(event: React.KeyboardEvent) => { + if ( + event.key === 'ArrowLeft' || + event.key === 'ArrowRight' || + event.key === 'ArrowUp' || + event.key === 'ArrowDown' + ) { + setIsKeyboardDrag(true) + onDragStart?.() + } + }} + onDoubleClick={onDoubleClick} + /> ) : null} - +
) } @@ -355,7 +336,7 @@ export type PageLayoutHeaderProps = { hidden?: boolean | ResponsiveValue className?: string style?: React.CSSProperties -} & SxProp +} const Header: React.FC> = ({ 'aria-label': label, @@ -366,7 +347,6 @@ const Header: React.FC> = ({ hidden = false, children, style, - sx, className, }) => { // Combine divider and dividerWhenNarrow for backwards compatibility @@ -380,12 +360,10 @@ const Header: React.FC> = ({ const {rowGap} = React.useContext(PageLayoutContext) return ( - + ) } @@ -443,7 +421,7 @@ export type PageLayoutContentProps = { hidden?: boolean | ResponsiveValue className?: string style?: React.CSSProperties -} & SxProp +} // TODO: Account for pane width when centering content // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -462,19 +440,17 @@ const Content: React.FC> = ({ padding = 'none', hidden = false, children, - sx, className, style, }) => { const isHidden = useResponsiveValue(hidden, false) + const Component = as return ( - @@ -489,7 +465,7 @@ const Content: React.FC> = ({ > {children}
- + ) } @@ -563,7 +539,7 @@ export type PageLayoutPaneProps = { id?: string className?: string style?: React.CSSProperties -} & SxProp +} // eslint-disable-next-line @typescript-eslint/no-unused-vars const panePositions = { @@ -601,7 +577,6 @@ const Pane = React.forwardRef - +

) }, ) @@ -803,7 +779,7 @@ export type PageLayoutFooterProps = { hidden?: boolean | ResponsiveValue className?: string style?: React.CSSProperties -} & SxProp +} const Footer: React.FC> = ({ 'aria-label': label, @@ -813,7 +789,6 @@ const Footer: React.FC> = ({ dividerWhenNarrow = 'inherit', hidden = false, children, - sx, className, style, }) => { @@ -828,13 +803,11 @@ const Footer: React.FC> = ({ const {rowGap} = React.useContext(PageLayoutContext) return ( - + ) } diff --git a/packages/react/src/UnderlineNav/UnderlineNav.docs.json b/packages/react/src/UnderlineNav/UnderlineNav.docs.json index 65b5a0da13f..aed639ccff1 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.docs.json +++ b/packages/react/src/UnderlineNav/UnderlineNav.docs.json @@ -55,11 +55,6 @@ "type": "'inset' | 'flush'", "defaultValue": "'inset'", "description": "`inset` children are offset horizontally from container edges. `flush` children are flush horizontally with container edges" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "subcomponents": [ @@ -100,11 +95,6 @@ "name": "as", "type": "React.ElementType", "defaultValue": "\"a\"" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "passthrough": { diff --git a/packages/react/src/UnderlineNav/UnderlineNav.module.css b/packages/react/src/UnderlineNav/UnderlineNav.module.css new file mode 100644 index 00000000000..79145f723f1 --- /dev/null +++ b/packages/react/src/UnderlineNav/UnderlineNav.module.css @@ -0,0 +1,77 @@ +.Divider { + display: inline-block; + border-left: var(--borderWidth-thin) solid var(--borderColor-muted); + width: var(--borderWidth-thin); + margin-right: var(--base-size-4); + height: 24px; /* The height of the divider - reference from Figma */ +} + +.MoreButton { + /* Set margin 0 here because safari puts extra margin around the button, + rest is to reset style to make it look like a list element */ + margin: 0; + border: 0; + font-weight: var(--base-text-weight-normal); + box-shadow: none; + padding-block: var(--base-size-4); + padding-inline: var(--base-size-8); +} + +.MoreButton, +.MoreButton:hover, +.MoreButton[aria-expanded='true'] { + background: transparent; +} + +.MoreButton[data-component='trailingVisual'] { + margin-left: 0; +} + +.MoreMenuListItem { + display: flex; + align-items: center; + /* Hard-coded height is needed to make sure we don't have a layout shift when the more button is the only item in the nav. */ + height: 45px; +} + +.MenuContainer { + position: absolute; + z-index: 1; + top: 90%; + box-shadow: + /* TODO: replace custom shadow with Primer Primitives variable */ + /* stylelint-disable-next-line primer/box-shadow */ + rgba(0, 0, 0, 0.12) 0 1px 3px, + rgba(0, 0, 0, 0.24) 0 1px 2px; + border-radius: var(--borderRadius-large); + background-color: var(--bgColor-default); + list-style: none; + min-width: 192px; /* baseMenuMinWidth */ + max-width: 640px; + right: 0; + display: block; +} + +.MenuContainer[data-is-widget-open='false'] { + display: none; +} + +.MenuItem { + text-decoration: none; +} + +.MenuItem > span:first-child { + display: none; +} + +.MenuItemContent { + display: flex; + align-items: center; + justify-content: space-between; +} + +.NavListItem { + display: flex; + flex-direction: column; + align-items: center; +} diff --git a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx index e205e043c99..b4ad1deb033 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx @@ -14,7 +14,7 @@ import { } from '@primer/octicons-react' import {UnderlineNav} from '.' -import {baseMenuMinWidth, menuStyles} from './styles' +import {BASE_MENU_MIN_WIDTH, getLeftAnchoredPosition} from './UnderlineNav' const ResponsiveUnderlineNav = ({ selectedItemText = 'Code', @@ -166,20 +166,20 @@ describe('UnderlineNav', () => { spy.mockRestore() }) - it(`menuStyles should set the menu position, if the container size is below ${baseMenuMinWidth} px`, () => { + it(`should set the menu position using getLeftAnchoredPosition, if the container size is below ${BASE_MENU_MIN_WIDTH} px`, () => { // GIVEN // Mock the refs. const containerRef = document.createElement('div') const listRef = document.createElement('div') // Set the clientWidth on the mock element - Object.defineProperty(listRef, 'clientWidth', {value: baseMenuMinWidth - 1}) + Object.defineProperty(listRef, 'clientWidth', {value: BASE_MENU_MIN_WIDTH - 1}) // WHEN - const results = menuStyles(containerRef, listRef) + const results = getLeftAnchoredPosition(containerRef, listRef) // THEN - // We are expecting a left value back, that way we know the `getAnchoredPosition` ran. - expect(results).toEqual(expect.objectContaining({left: 0})) + // We are expecting a left value back, that way we know the `getLeftAnchoredPosition` ran. + expect(results).toEqual(0) }) it('should support icons passed in as an element', () => { diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index c0f44fe244c..8b64fc8eea1 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -1,33 +1,28 @@ import type {MutableRefObject, RefObject} from 'react' import React, {useRef, forwardRef, useCallback, useState, useEffect} from 'react' -import Box from '../Box' -import type {SxProp} from '../sx' -import sx from '../sx' +import {getAnchoredPosition} from '@primer/behaviors' import {UnderlineNavContext} from './UnderlineNavContext' import type {ResizeObserverEntry} from '../hooks/useResizeObserver' import {useResizeObserver} from '../hooks/useResizeObserver' import {useTheme} from '../ThemeProvider' import type {ChildWidthArray, ResponsiveProps, ChildSize} from './types' import VisuallyHidden from '../_VisuallyHidden' -import {moreBtnStyles, getDividerStyle, menuStyles, menuItemStyles, baseMenuStyles, baseMenuMinWidth} from './styles' import {UnderlineItemList, UnderlineWrapper, LoadingCounter, GAP} from '../internal/components/UnderlineTabbedInterface' -import styled from 'styled-components' import {Button} from '../Button' import {TriangleDownIcon} from '@primer/octicons-react' import {useOnEscapePress} from '../hooks/useOnEscapePress' import {useOnOutsideClick} from '../hooks/useOnOutsideClick' import {useId} from '../hooks/useId' import {ActionList} from '../ActionList' -import {defaultSxProp} from '../utils/defaultSxProp' import CounterLabel from '../CounterLabel' import {invariant} from '../utils/invariant' +import classes from './UnderlineNav.module.css' export type UnderlineNavProps = { children: React.ReactNode 'aria-label'?: React.AriaAttributes['aria-label'] as?: React.ElementType className?: string - sx?: SxProp['sx'] /** * loading state for all counters. It displays loading animation for individual counters (UnderlineNav.Item) until all are resolved. It is needed to prevent multiple layout shift. */ @@ -42,19 +37,7 @@ export type UnderlineNavProps = { // When page is loaded, we don't have ref for the more button as it is not on the DOM yet. // However, we need to calculate number of possible items when the more button present as well. So using the width of the more button as a constant. export const MORE_BTN_WIDTH = 86 -// The height is needed to make sure we don't have a layout shift when the more button is the only item in the nav. -const MORE_BTN_HEIGHT = 45 - -// Needed this because passing a ref using HTMLULListElement to `Box` causes a type error -export const NavigationList = styled.ul` - ${sx}; -` - -export const MoreMenuListItem = styled.li` - display: flex; - align-items: center; - height: ${MORE_BTN_HEIGHT}px; -` +export const BASE_MENU_MIN_WIDTH = 192 const overflowEffect = ( navWidth: number, @@ -140,12 +123,25 @@ const calculatePossibleItems = (childWidthArray: ChildWidthArray, navWidth: numb return breakpoint } +/** + * + * @param containerRef The Menu List Container Reference. + * @param listRef The Underline Nav Container Reference. + * @description This calculates the position of the menu + * + * Exported because it's used in unit tests. + */ +export const getLeftAnchoredPosition = (containerRef: Element | null, listRef: Element | null): number => { + return containerRef && listRef + ? getAnchoredPosition(containerRef, listRef, {align: 'start', side: 'outside-bottom'}).left + : 0 +} + export const UnderlineNav = forwardRef( ( { as = 'nav', 'aria-label': ariaLabel, - sx: sxProp = defaultSxProp, loadingCounters = false, variant = 'inset', className, @@ -316,28 +312,21 @@ export const UnderlineNav = forwardRef( }} > {ariaLabel && {`${ariaLabel} navigation`}} - + {listItems} {menuItems.length > 0 && ( - - {!onlyMenuVisible && } +
  • + {!onlyMenuVisible &&
    } = baseMenuMinWidth - ? baseMenuStyles - : menuStyles(containerRef.current, listRef.current) - } - style={{display: isWidgetOpen ? 'block' : 'none'}} + className={classes.MenuContainer} + data-is-widget-open={isWidgetOpen} + style={{ + left: + listRef.current?.clientWidth && listRef.current.clientWidth >= BASE_MENU_MIN_WIDTH + ? undefined + : getLeftAnchoredPosition(containerRef.current, listRef.current), + }} > {menuItems.map((menuItem, index) => { const { @@ -385,7 +376,7 @@ export const UnderlineNav = forwardRef( return ( | React.KeyboardEvent, ) => { @@ -398,23 +389,23 @@ export const UnderlineNav = forwardRef( }} {...menuItemProps} > - + {menuItemChildren} {loadingCounters ? ( ) : ( counter !== undefined && ( - + {counter} - + ) )} - + ) })} - +
    • )}     diff --git a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx index f63c38c270b..31430769097 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx @@ -1,13 +1,11 @@ import type {MutableRefObject, RefObject} from 'react' import React, {forwardRef, useRef, useContext} from 'react' -import Box from '../Box' -import type {SxProp} from '../sx' import type {IconProps} from '@primer/octicons-react' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {UnderlineNavContext} from './UnderlineNavContext' import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' -import {defaultSxProp} from '../utils/defaultSxProp' import {UnderlineItem} from '../internal/components/UnderlineTabbedInterface' +import classes from './UnderlineNav.module.css' // adopted from React.AnchorHTMLAttributes export type LinkProps = { @@ -47,22 +45,11 @@ export type UnderlineNavItemProps = { * Counter */ counter?: number | string -} & SxProp & - LinkProps +} & LinkProps export const UnderlineNavItem = forwardRef( ( - { - sx: sxProp = defaultSxProp, - as: Component = 'a', - href = '#', - children, - counter, - onSelect, - 'aria-current': ariaCurrent, - icon: Icon, - ...props - }, + {as: Component = 'a', href = '#', children, counter, onSelect, 'aria-current': ariaCurrent, icon: Icon, ...props}, forwardedRef, ) => { const backupRef = useRef(null) @@ -111,7 +98,7 @@ export const UnderlineNavItem = forwardRef( ) return ( - +
  • {children} - +
    • ) }, + // TODO: replace this with new way to handle polymorphic prop types from `packages/react/src/utils/modern-polymorphic.ts` ) as PolymorphicForwardRefComponent<'a', UnderlineNavItemProps> UnderlineNavItem.displayName = 'UnderlineNavItem' diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 2d2c5365d04..9e593f99f13 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -117,6 +117,7 @@ export type { NavListLeadingVisualProps, NavListTrailingVisualProps, NavListDividerProps, + NavListGroupHeadingProps, } from './NavList' export {default as Overlay} from './Overlay' export type {OverlayProps} from './Overlay' @@ -203,7 +204,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..73dfc4afae7 --- /dev/null +++ b/packages/react/src/utils/modern-polymorphic.ts @@ -0,0 +1,30 @@ +// 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 + */ +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 = DistributiveOmit< + ComponentPropsWithRef, + 'as' +> diff --git a/packages/styled-react/src/__tests__/primer-react.browser.test.tsx b/packages/styled-react/src/__tests__/primer-react.browser.test.tsx index 7909455e5ae..dd2add4cc82 100644 --- a/packages/styled-react/src/__tests__/primer-react.browser.test.tsx +++ b/packages/styled-react/src/__tests__/primer-react.browser.test.tsx @@ -176,9 +176,118 @@ describe('@primer/react', () => { expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) - test('Heading supports `sx` prop', () => { - render() - expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') + const theme = { + breakpoints: ['400px', '640px', '960px', '1280px'], + colors: { + green: ['#010', '#020', '#030', '#040', '#050', '#060'], + }, + fontSizes: ['12px', '14px', '16px', '20px', '24px', '32px', '40px', '48px'], + fonts: { + normal: 'Helvetica,sans-serif', + mono: 'Consolas,monospace', + }, + lineHeights: { + normal: 1.5, + condensed: 1.25, + condensedUltra: 1, + }, + fontWeights: { + light: '300', + normal: '400', + semibold: '500', + bold: '600', + }, + } + test('Heading supports `fontWeight` in `sx` prop', () => { + const {container} = render( + + + , + ) + const heading = container.firstChild as HTMLElement + expect(heading).toHaveStyle(`font-weight: ${theme.fontWeights.bold}`) + + const {container: container2} = render( + + + , + ) + const heading2 = container2.firstChild as HTMLElement + expect(heading2).toHaveStyle(`font-weight: ${theme.fontWeights.normal}`) + + const {container: container3} = render( + + + , + ) + const heading3 = container3.firstChild as HTMLElement + expect(heading3).toHaveStyle(`font-weight: ${theme.fontWeights.semibold}`) + + const {container: container4} = render( + + + , + ) + const heading4 = container4.firstChild as HTMLElement + expect(heading4).toHaveStyle(`font-weight: ${theme.fontWeights.light}`) + }) + + test('Heading supports `lineHeight` in `sx` prop', () => { + const {container} = render( + + + , + ) + const heading = container.firstChild as HTMLElement + ///These sx tests should go away right? + expect(heading).toHaveStyle(`line-height: 48px`) + + const {container: container2} = render( + + + , + ) + const heading2 = container2.firstChild as HTMLElement + expect(heading2).toHaveStyle(`line-height: 40px`) + + const {container: container3} = render( + + + , + ) + const heading3 = container3.firstChild as HTMLElement + expect(heading3).toHaveStyle(`line-height: 32px`) + }) + + test('Heading supports `fontFamily` in `sx` prop', () => { + const {container} = render( + + + , + ) + const heading = container.firstChild as HTMLElement + expect(heading).toHaveStyle(`font-family: ${theme.fonts.mono}`) + }) + + test('Heading supports `fontSize` in `sx` prop', () => { + for (const fontSize of theme.fontSizes) { + const {container} = render( + + + , + ) + const heading = container.firstChild as HTMLElement + expect(heading).toHaveStyle(`font-size: ${fontSize}`) + } + }) + test('Heading supports `fontStyle` in `sx` prop', () => { + const {container} = render( + + + , + ) + const heading = container.firstChild as HTMLElement + expect(heading).toHaveStyle('font-style: italic') }) test('IconButton supports `sx` prop', () => { @@ -210,7 +319,8 @@ describe('@primer/react', () => { expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) - test.todo('NavList.Item supports `sx` prop', () => { + // TODO: figure out why `sx` isn't working here + test('NavList.Item supports `sx` prop', () => { render( @@ -218,12 +328,36 @@ describe('@primer/react', () => { , ) - expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') + + const itemAnchorEl = screen.getByTestId('component') + const itemLiEl = itemAnchorEl.closest('li') + expect(itemLiEl).not.toBeNull() + expect(window.getComputedStyle(itemLiEl!).backgroundColor).toBe('rgb(255, 0, 0)') }) test('NavList.Group supports `sx` prop', () => { - const {container} = render(test) - expect(window.getComputedStyle(container.firstElementChild!).backgroundColor).toBe('rgb(255, 0, 0)') + render( + + + item + + , + ) + expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') + }) + + test('NavList.GroupHeading supports `sx` prop', () => { + render( + + + + test + + item + + , + ) + expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) test('NavList.LeadingVisual supports `sx` prop', () => { @@ -231,6 +365,26 @@ describe('@primer/react', () => { expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) + // TODO: figure out why `sx` isn't working here + test('NavList.SubNav supports `sx` prop', () => { + render( + + + Parent item + + subitem + + + , + ) + + // Select the NavList.SubNav element by finding the "subitem" text and traversing up to the nearest
      + const itemAnchorEl = screen.getByTestId('component') + const subNavElement = itemAnchorEl.closest('ul') + expect(subNavElement).not.toBeNull() + expect(window.getComputedStyle(subNavElement!).backgroundColor).toBe('rgb(255, 0, 0)') + }) + test('Overlay supports `sx` prop', () => { const ref = createRef() render( @@ -352,12 +506,12 @@ describe('@primer/react', () => { expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) - test('SubNav supports `sx` prop', () => { + test.skip('SubNav supports `sx` prop', () => { render() expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) - test('SubNav.Link supports `sx` prop', () => { + test.skip('SubNav.Link supports `sx` prop', () => { render() expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) diff --git a/packages/styled-react/src/components/NavList.tsx b/packages/styled-react/src/components/NavList.tsx new file mode 100644 index 00000000000..8b803dfac7a --- /dev/null +++ b/packages/styled-react/src/components/NavList.tsx @@ -0,0 +1,73 @@ +import {NavList as PrimerNavList, Box} from '@primer/react' +import type { + NavListProps as PrimerNavListProps, + NavListItemProps as PrimerNavListItemProps, + NavListSubNavProps as PrimerNavListSubNavProps, + NavListDividerProps as PrimerNavListDividerProps, + NavListGroupProps as PrimerNavListGroupProps, + NavListGroupHeadingProps as PrimerNavListGroupHeadingProps, + SxProp, +} from '@primer/react' +import {forwardRef, type PropsWithChildren} from 'react' + +type NavListProps = PropsWithChildren & SxProp + +const NavListImpl = forwardRef(function NavList(props, ref) { + return +}) + +type NavListItemProps = PropsWithChildren & SxProp + +const NavListItem = forwardRef(function NavListItem(props, ref) { + // @ts-expect-error - PrimerNavList.Item is not recognized as a valid component type + return +}) + +type NavListSubNavProps = PropsWithChildren & SxProp + +const NavListSubNav = forwardRef(function NavListSubNav(props, ref) { + // @ts-expect-error - PrimerNavList.SubNav is not recognized as a valid component type + return +}) + +type NavListDividerProps = PropsWithChildren & SxProp + +const NavListDivider = forwardRef(function NavListDivider(props, ref) { + // @ts-expect-error - PrimerNavList.Divider is not recognized as a valid component type + return +}) + +type NavListGroupProps = PropsWithChildren & SxProp + +const NavListGroup = forwardRef(function NavListGroup(props, ref) { + // @ts-expect-error - PrimerNavList.Group is not recognized as a valid component type + return +}) + +type NavListGroupHeadingProps = PropsWithChildren & SxProp + +// TODO: figure out how to handle `NavList.GroupHeading`'s `as` prop +const NavListGroupHeading = forwardRef( + function NavListGroupHeading(props, ref) { + return + }, +) + +// TODO: figure out why we need a type assertion here and try to remove it +const NavList = Object.assign(NavListImpl, { + // Wrapped components that need sx support added back in + Item: NavListItem, + SubNav: NavListSubNav, + Divider: NavListDivider, + Group: NavListGroup, + GroupHeading: NavListGroupHeading, + + // Re-exporting others directly + LeadingVisual: PrimerNavList.LeadingVisual, + TrailingVisual: PrimerNavList.TrailingVisual, + TrailingAction: PrimerNavList.TrailingAction, + GroupExpand: PrimerNavList.GroupExpand, +}) + +export {NavList} +export type {NavListProps} diff --git a/packages/styled-react/src/components/PageHeader.tsx b/packages/styled-react/src/components/PageHeader.tsx new file mode 100644 index 00000000000..eec81bb5a75 --- /dev/null +++ b/packages/styled-react/src/components/PageHeader.tsx @@ -0,0 +1,52 @@ +import React, {type PropsWithChildren} from 'react' +import type { + PageHeaderProps as PrimerPageHeaderProps, + ParentLinkProps as PrimerParentLinkProps, + TitleProps as PrimerTitleProps, +} from '@primer/react' +import {PageHeader as PrimerPageHeader, Box} from '@primer/react' +import type {SxProp} from '../sx' + +type PageHeaderProps = PropsWithChildren & SxProp + +const PageHeaderImpl = React.forwardRef((props, ref) => { + return +}) + +type PageHeaderTitleProps = PropsWithChildren & SxProp + +// Create wrapped versions of components that need sx support +const PageHeaderTitle = React.forwardRef((props, ref) => { + // @ts-expect-error - PrimerPageHeader.Title is not recognized as a valid component type + return +}) + +type PageHeaderParentLinkProps = PropsWithChildren & SxProp + +const PageHeaderParentLink = React.forwardRef((props, ref) => { + return +}) + +// TODO: figure out why we need a type assertion here and try to remove it +const PageHeader = Object.assign(PageHeaderImpl, { + // 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/components/PageLayout.tsx b/packages/styled-react/src/components/PageLayout.tsx new file mode 100644 index 00000000000..f1b02094601 --- /dev/null +++ b/packages/styled-react/src/components/PageLayout.tsx @@ -0,0 +1,58 @@ +import React, {type PropsWithChildren} from 'react' +import styled from 'styled-components' +import type { + PageLayoutProps as PrimerPageLayoutProps, + PageLayoutContentProps as PrimerPageLayoutContentProps, + PageLayoutHeaderProps as PrimerPageLayoutHeaderProps, + PageLayoutPaneProps as PrimerPageLayoutPaneProps, + PageLayoutFooterProps as PrimerPageLayoutFooterProps, +} from '@primer/react' +import {PageLayout as PrimerPageLayout} from '@primer/react' +import {sx, type SxProp} from '../sx' + +const Wrapper = styled.div` + ${sx} +` + +type PageLayoutProps = PropsWithChildren & SxProp + +const PageLayoutImpl = React.forwardRef((props, ref) => { + // @ts-expect-error - PrimerPageLayout is not recognized as a valid component type + return +}) + +type PageLayoutContentProps = PropsWithChildren & SxProp + +const PageLayoutContent = React.forwardRef((props, ref) => { + return +}) + +type PageLayoutHeaderProps = PropsWithChildren & SxProp + +const PageLayoutHeader = React.forwardRef((props, ref) => { + // @ts-expect-error - PrimerPageLayout.Header is not recognized as a valid component type + return +}) + +type PageLayoutPaneProps = PropsWithChildren & SxProp + +const PageLayoutPane = React.forwardRef((props, ref) => { + return +}) + +type PageLayoutFooterProps = PropsWithChildren + +const PageLayoutFooter = React.forwardRef((props, ref) => { + // @ts-expect-error - PrimerPageLayout.Footer is not recognized as a valid component type + return +}) + +const PageLayout = Object.assign(PageLayoutImpl, { + Content: PageLayoutContent, + Header: PageLayoutHeader, + Pane: PageLayoutPane, + Footer: PageLayoutFooter, +}) + +export {PageLayout} +export type {PageLayoutProps} diff --git a/packages/styled-react/src/components/UnderlineNav.tsx b/packages/styled-react/src/components/UnderlineNav.tsx new file mode 100644 index 00000000000..09cd89d8d28 --- /dev/null +++ b/packages/styled-react/src/components/UnderlineNav.tsx @@ -0,0 +1,27 @@ +import React, {type PropsWithChildren} from 'react' +import {UnderlineNav as PrimerUnderlineNav, Box} from '@primer/react' +import type { + UnderlineNavProps as PrimerUnderlineNavProps, + UnderlineNavItemProps as PrimerUnderlineNavItemProps, +} from '@primer/react' +import type {SxProp} from '../sx' + +type UnderlineNavProps = PropsWithChildren & SxProp + +const UnderlineNavImpl = React.forwardRef((props, ref) => { + return +}) + +type UnderlineNavItemProps = PropsWithChildren & SxProp + +const UnderlineNavItem = React.forwardRef((props, ref) => { + return +}) + +// TODO: figure out why we need a type assertion here and try to remove it +const UnderlineNav = Object.assign(UnderlineNavImpl, { + Item: UnderlineNavItem, +}) + +export {UnderlineNav} +export type {UnderlineNavProps, UnderlineNavItemProps} diff --git a/packages/styled-react/src/index.tsx b/packages/styled-react/src/index.tsx index bb1405345f0..ec2774dfd80 100644 --- a/packages/styled-react/src/index.tsx +++ b/packages/styled-react/src/index.tsx @@ -2,7 +2,6 @@ import { type BetterSystemStyleObject, Box, type BoxProps, - type SxProp, StateLabel as PrimerStateLabel, type StateLabelProps as PrimerStateLabelProps, SubNav as PrimerSubNav, @@ -10,6 +9,12 @@ import { type SubNavLinkProps as PrimerSubNavLinkProps, ToggleSwitch as PrimerToggleSwitch, type ToggleSwitchProps as PrimerToggleSwitchProps, + Link as PrimerLink, + type LinkProps as PrimerLinkProps, + Heading as PrimerHeading, + type HeadingProps as PrimerHeadingProps, + Checkbox as PrimerCheckbox, + type CheckboxProps as PrimerCheckboxProps, } from '@primer/react' import {forwardRef} from 'react' import type { @@ -24,6 +29,11 @@ import type { SpaceProps, TypographyProps, } from 'styled-system' +import {type SxProp} from './sx' +import {PageHeader} from './components/PageHeader' +import {PageLayout} from './components/PageLayout' +import {NavList} from './components/NavList' +import {UnderlineNav} from './components/UnderlineNav' type StyledProps = SxProp & SpaceProps & @@ -65,7 +75,25 @@ const ToggleSwitch = forwardRef(function T return }) -export {StateLabel, SubNav, ToggleSwitch} +type LinkProps = PrimerLinkProps & SxProp + +const Link = forwardRef(function Link(props, ref) { + return +}) + +type HeadingProps = PrimerHeadingProps & SxProp + +const Heading = forwardRef(function Heading(props, ref) { + return +}) + +type CheckboxProps = PrimerCheckboxProps & SxProp + +const Checkbox = forwardRef(function Checkbox(props, ref) { + return +}) + +export {Checkbox, Heading, Link, NavList, PageHeader, PageLayout, StateLabel, SubNav, ToggleSwitch, UnderlineNav} export { ActionList, @@ -74,7 +102,6 @@ export { Avatar, Breadcrumbs, Button, - Checkbox, CheckboxGroup, CircleBadge, CounterLabel, @@ -83,15 +110,10 @@ export { Flash, FormControl, Header, - Heading, IconButton, Label, - Link, LinkButton, - NavList, Overlay, - PageHeader, - PageLayout, Popover, ProgressBar, RadioGroup, @@ -106,7 +128,6 @@ export { Token, Tooltip, Truncate, - UnderlineNav, // styled-components components or types Box, 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} From 778eca12d6c6c132aff98e8b454235b7cae44ae2 Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Wed, 10 Sep 2025 17:40:39 -0400 Subject: [PATCH 02/47] fixes NavList/ActionList type errors surfaced when creating wrappers in styled-react --- packages/react/src/ActionList/Item.tsx | 49 ++++--- packages/react/src/ActionList/List.tsx | 122 +++++++++--------- packages/react/src/NavList/NavList.tsx | 2 + .../src/SegmentedControl/SegmentedControl.tsx | 2 +- .../styled-react/src/components/NavList.tsx | 34 ++--- 5 files changed, 113 insertions(+), 96 deletions(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 49944d74fe2..638a279697e 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -2,7 +2,6 @@ import React from 'react' import {useId} from '../hooks/useId' import {useSlots} from '../hooks/useSlots' -import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {ActionListContainerContext} from './ActionListContainerContext' import {Description} from './Description' import {GroupContext} from './Group' @@ -28,23 +27,27 @@ export const SubItem: React.FC = ({children}) => { SubItem.displayName = 'ActionList.SubItem' -const ButtonItemContainerNoBox = React.forwardRef(({children, style, ...props}, forwardedRef) => { - return ( - - ) -}) as PolymorphicForwardRefComponent - -const DivItemContainerNoBox = React.forwardRef(({children, ...props}, forwardedRef) => { - return ( -
      } {...props}> - {children} -
      - ) -}) as PolymorphicForwardRefComponent - -export const Item = React.forwardRef( +const ButtonItemContainerNoBox = React.forwardRef>( + ({children, style, ...props}, forwardedRef) => { + return ( + + ) + }, +) + +const DivItemContainerNoBox = React.forwardRef>( + ({children, ...props}, forwardedRef) => { + return ( +
      } {...props}> + {children} +
      + ) + }, +) + +export const Item = React.forwardRef>( ( { variant = 'default', @@ -254,7 +257,13 @@ export const Item = React.forwardRef( data-has-description={slots.description ? true : false} className={clsx(classes.ActionListItem, className)} > - + ( ) }, -) as PolymorphicForwardRefComponent<'li', ActionListItemProps> +) Item.displayName = 'ActionList.Item' diff --git a/packages/react/src/ActionList/List.tsx b/packages/react/src/ActionList/List.tsx index f8448ba0055..81b71321d4e 100644 --- a/packages/react/src/ActionList/List.tsx +++ b/packages/react/src/ActionList/List.tsx @@ -1,5 +1,5 @@ import React from 'react' -import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {fixedForwardRef} from '../utils/modern-polymorphic' import {ActionListContainerContext} from './ActionListContainerContext' import {useSlots} from '../hooks/useSlots' import {Heading} from './Heading' @@ -11,68 +11,74 @@ import {clsx} from 'clsx' import classes from './ActionList.module.css' import {BoxWithFallback} from '../internal/components/BoxWithFallback' -export const List = React.forwardRef( - ( - {variant = 'inset', selectionVariant, showDividers = false, role, disableFocusZone = false, className, ...props}, - forwardedRef, - ): JSX.Element => { - const [slots, childrenWithoutSlots] = useSlots(props.children, { - heading: Heading, - }) +const UnwrappedList = (props: ActionListProps, forwardedRef: React.ForwardedRef): JSX.Element => { + const { + variant = 'inset', + selectionVariant, + showDividers = false, + role, + disableFocusZone = false, + className, + ...restProps + } = props + const [slots, childrenWithoutSlots] = useSlots(restProps.children, { + heading: Heading, + }) - const headingId = useId() + const headingId = useId() - /** if list is inside a Menu, it will get a role from the Menu */ - const { - listRole: listRoleFromContainer, - listLabelledBy, - selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation - enableFocusZone: enableFocusZoneFromContainer, - container, - } = React.useContext(ActionListContainerContext) + /** if list is inside a Menu, it will get a role from the Menu */ + const { + listRole: listRoleFromContainer, + listLabelledBy, + selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation + enableFocusZone: enableFocusZoneFromContainer, + container, + } = React.useContext(ActionListContainerContext) - const ariaLabelledBy = slots.heading ? (slots.heading.props.id ?? headingId) : listLabelledBy - const listRole = role || listRoleFromContainer - const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) + const ariaLabelledBy = slots.heading ? (slots.heading.props.id ?? headingId) : listLabelledBy + const listRole = role || listRoleFromContainer + const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) - let enableFocusZone = false - if (enableFocusZoneFromContainer !== undefined) enableFocusZone = enableFocusZoneFromContainer - else if (listRole && !disableFocusZone) enableFocusZone = ['menu', 'menubar', 'listbox'].includes(listRole) + let enableFocusZone = false + if (enableFocusZoneFromContainer !== undefined) enableFocusZone = enableFocusZoneFromContainer + else if (listRole && !disableFocusZone) enableFocusZone = ['menu', 'menubar', 'listbox'].includes(listRole) - useFocusZone({ - disabled: !enableFocusZone, - containerRef: listRef, - bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown, - focusOutBehavior: - listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined, - }) + useFocusZone({ + disabled: !enableFocusZone, + containerRef: listRef, + bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown, + focusOutBehavior: + listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined, + }) - return ( - + {slots.heading} + - {slots.heading} - - {childrenWithoutSlots} - - - ) - }, -) as PolymorphicForwardRefComponent<'ul', ActionListProps> + {childrenWithoutSlots} + + + ) +} -List.displayName = 'ActionList' +export const List = fixedForwardRef(UnwrappedList) + +Object.assign(List, {displayName: 'ActionList'}) diff --git a/packages/react/src/NavList/NavList.tsx b/packages/react/src/NavList/NavList.tsx index fa75e61834e..8acb4c5633b 100644 --- a/packages/react/src/NavList/NavList.tsx +++ b/packages/react/src/NavList/NavList.tsx @@ -325,6 +325,8 @@ export const GroupExpand = React.forwardRef