Skip to content

Remove sx from UnderlinePanels #6874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
francinelucca merged 18 commits into from
Sep 30, 2025
Merged

Remove sx from UnderlinePanels #6874

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9548fcc
Fix up panels
pksjce Sep 10, 2025
7e4132e
not a good state
pksjce Sep 17, 2025
54c09fb
Fix the underlineitem types
pksjce Sep 18, 2025
64d1fc9
Create chilly-lemons-promise.md
pksjce Sep 18, 2025
82a68db
Fix types
pksjce Sep 18, 2025
da12e6e
Remove the browser test
pksjce Sep 18, 2025
a868c4d
Good catch by the tests
pksjce Sep 18, 2025
842347d
UnderlineNav.Item is sx free
pksjce Sep 18, 2025
aec771a
Remove UnderlineNav from test
pksjce Sep 19, 2025
b4bd28b
Merge branch 'main' of github.com:primer/react into pk/remove-sx-unde…
francinelucca Sep 25, 2025
891bcf8
re-add tests
francinelucca Sep 25, 2025
6587d22
add underlinePanels
francinelucca Sep 25, 2025
24c9bdf
remove console.log
francinelucca Sep 25, 2025
163e7bd
fix types
francinelucca Sep 26, 2025
4962e4f
remove unnecessary any
francinelucca Sep 26, 2025
8850160
Merge branch 'main' of github.com:primer/react into pk/remove-sx-unde…
francinelucca Sep 29, 2025
0578bcb
remove ts-ignore
francinelucca Sep 30, 2025
eb95b53
Merge branch 'main' into pk/remove-sx-underline-panels
francinelucca Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,14 @@ import React, {
import {TabContainerElement} from '@github/tab-container-element'
import type {IconProps} from '@primer/octicons-react'
import {createComponent} from '../../utils/create-component'
import {
UnderlineItemList,
UnderlineWrapper,
UnderlineItem,
type UnderlineItemProps,
} from '../../internal/components/UnderlineTabbedInterface'
import {type BoxProps} from '../../Box'
import {UnderlineItemList, UnderlineWrapper, UnderlineItem} from '../../internal/components/UnderlineTabbedInterface'
import {useId} from '../../hooks'
import {invariant} from '../../utils/invariant'
import {type SxProp} from '../../sx'
import {useResizeObserver, type ResizeObserverEntry} from '../../hooks/useResizeObserver'
import useIsomorphicLayoutEffect from '../../utils/useIsomorphicLayoutEffect'
import classes from './UnderlinePanels.module.css'
import {clsx} from 'clsx'
import {BoxWithFallback} from '../../internal/components/BoxWithFallback'

export type UnderlinePanelsProps = {
/**
Expand Down Expand Up @@ -74,7 +67,7 @@ export type TabProps = PropsWithChildren<{
}> &
SxProp

export type PanelProps = Omit<BoxProps, 'as'>
export type PanelProps = React.HTMLAttributes<HTMLDivElement>

const TabContainerComponent = createComponent(TabContainerElement, 'tab-container')

Expand Down Expand Up @@ -104,7 +97,7 @@ const UnderlinePanels: FC<UnderlinePanelsProps> = ({
let panelIndex = 0

const childrenWithProps = Children.map(children, child => {
if (isValidElement<UnderlineItemProps>(child) && child.type === Tab) {
if (isValidElement(child) && child.type === Tab) {
return cloneElement(child, {id: `${parentId}-tab-${tabIndex++}`, loadingCounters, iconsVisible})
}

Expand Down Expand Up @@ -221,8 +214,12 @@ const Tab: FC<TabProps> = ({'aria-selected': ariaSelected, onSelect, ...props})

Tab.displayName = 'UnderlinePanels.Tab'

const Panel: FC<PanelProps> = props => {
return <BoxWithFallback as="div" role="tabpanel" {...props} />
const Panel: FC<PanelProps> = ({children, ...rest}) => {
return (
<div role="tabpanel" {...rest}>
{children}
</div>
)
}

Panel.displayName = 'UnderlinePanels.Panel'
Expand Down
83 changes: 34 additions & 49 deletions packages/react/src/internal/components/UnderlineTabbedInterface.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
// Used for UnderlineNav and UnderlinePanels components

import type React from 'react'
import {forwardRef, type FC, type PropsWithChildren} from 'react'
import React from 'react'
import {forwardRef, type FC, type PropsWithChildren, type ElementType} from 'react'
import {isElement} from 'react-is'
import type {IconProps} from '@primer/octicons-react'
import CounterLabel from '../../CounterLabel'
import {type SxProp} from '../../sx'
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../../utils/polymorphic'

import classes from './UnderlineTabbedInterface.module.css'
import {clsx} from 'clsx'
import {BoxWithFallback} from './BoxWithFallback'

// The gap between the list items. It is a constant because the gap is used to calculate the possible number of items that can fit in the container.
export const GAP = 8
Expand All @@ -19,15 +17,15 @@ type UnderlineWrapperProps = {
slot?: string
as?: React.ElementType
className?: string
ref?: React.Ref<unknown>
} & SxProp
ref?: React.Ref<HTMLDivElement>
}

export const UnderlineWrapper = forwardRef(
({children, className, ...rest}: PropsWithChildren<UnderlineWrapperProps>, forwardedRef) => {
export const UnderlineWrapper = forwardRef<HTMLDivElement, PropsWithChildren<UnderlineWrapperProps>>(
({children, className, ...rest}, forwardedRef) => {
return (
<BoxWithFallback className={clsx(classes.UnderlineWrapper, className)} ref={forwardedRef} {...rest}>
<div className={clsx(classes.UnderlineWrapper, className)} ref={forwardedRef} {...rest}>
{children}
</BoxWithFallback>
</div>
)
},
)
Expand All @@ -44,51 +42,38 @@ export const LoadingCounter = () => {
return <span className={classes.LoadingCounter} />
}

export type UnderlineItemProps = {
as?: React.ElementType | 'a' | 'button'
export type UnderlineItemProps<As extends React.ElementType> = {
as?: As | 'a' | 'button'
className?: string
iconsVisible?: boolean
loadingCounters?: boolean
counter?: number | string
icon?: FC<IconProps> | React.ReactElement
id?: string
ref?: React.Ref<unknown>
} & SxProp
} & React.ComponentPropsWithoutRef<As extends 'a' ? 'a' : As extends 'button' ? 'button' : As>

@hectahertz hectahertz Sep 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😵‍💫


export const UnderlineItem = forwardRef(
(
{
as = 'a',
children,
counter,
icon: Icon,
iconsVisible,
loadingCounters,
className,
...rest
}: PropsWithChildren<UnderlineItemProps>,
forwardedRef,
) => {
return (
<BoxWithFallback ref={forwardedRef} as={as} className={clsx(classes.UnderlineItem, className)} {...rest}>
{iconsVisible && Icon && <span data-component="icon">{isElement(Icon) ? Icon : <Icon />}</span>}
{children && (
<span data-component="text" data-content={children}>
{children}
export const UnderlineItem = React.forwardRef((props, ref) => {
const {as: Component = 'a', children, counter, icon: Icon, iconsVisible, loadingCounters, className, ...rest} = props
Comment on lines +59 to +60

Copilot AI Sep 18, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The forwardRef should include proper TypeScript generics and parameter names for better type inference and debugging. Consider using the same pattern as UnderlineWrapper with explicit generic parameters.

See below for a potential fix:

export const UnderlineItem = React.forwardRef<
  HTMLElement,
  UnderlineItemProps<ElementType>
>(
  (props, ref) => {
    const {as: Component = 'a', children, counter, icon: Icon, iconsVisible, loadingCounters, className, ...rest} = props
    return (
      <Component {...rest} ref={ref} className={clsx(classes.UnderlineItem, className)}>
        {iconsVisible && Icon && <span data-component="icon">{isElement(Icon) ? Icon : <Icon />}</span>}
        {children && (
          <span data-component="text" data-content={children}>
            {children}
          </span>
        )}
        {counter !== undefined ? (
          loadingCounters ? (
            <span data-component="counter">
              <LoadingCounter />
            </span>
          ) : (
            <span data-component="counter">
              <CounterLabel>{counter}</CounterLabel>
            </span>
          )
        ) : null}
      </Component>
    )
  }
)

Copilot uses AI. Check for mistakes.
return (
<Component {...rest} ref={ref} className={clsx(classes.UnderlineItem, className)}>
{iconsVisible && Icon && <span data-component="icon">{isElement(Icon) ? Icon : <Icon />}</span>}
{children && (
<span data-component="text" data-content={children}>
{children}
</span>
)}
{counter !== undefined ? (
loadingCounters ? (
<span data-component="counter">
<LoadingCounter />
</span>
)}
{counter !== undefined ? (
loadingCounters ? (
<span data-component="counter">
<LoadingCounter />
</span>
) : (
<span data-component="counter">
<CounterLabel>{counter}</CounterLabel>
</span>
)
) : null}
</BoxWithFallback>
)
},
) as PolymorphicForwardRefComponent<'a', UnderlineItemProps>
) : (
<span data-component="counter">
<CounterLabel>{counter}</CounterLabel>
</span>
)
) : null}
</Component>
)
}) as PolymorphicForwardRefComponent<ElementType, UnderlineItemProps<ElementType>>

Copilot AI Sep 18, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type assertion uses ElementType as both the constraint and the argument, which may not provide the intended polymorphic behavior. Consider using a more specific constraint like keyof JSX.IntrinsicElements | React.ComponentType<any> to ensure proper component props are inferred.

Suggested change
}) as PolymorphicForwardRefComponent<ElementType, UnderlineItemProps<ElementType>>
}) as PolymorphicForwardRefComponent<keyof JSX.IntrinsicElements | React.ComponentType<any>, UnderlineItemProps<any>>

Copilot uses AI. Check for mistakes.
Loading