Remove sx from UnderlinePanels #6874
Conversation
Co-authored-by: Marie Lucca <[email protected]>
🦋 Changeset detectedLatest commit: aec771a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the sx prop and SxProp type dependencies from the UnderlinePanels component, replacing the styled BoxWithFallback components with standard HTML elements (div). This change is part of a larger effort to remove the sx prop system from Primer React components.
Key changes:
- Replaces
BoxWithFallbackusage with native HTML elements - Updates type definitions to remove
SxPropdependencies - Improves type safety with better generic constraints for polymorphic components
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/internal/components/UnderlineTabbedInterface.tsx | Removes SxProp imports and BoxWithFallback usage, replaces with standard div elements and improves TypeScript generics |
| packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx | Updates Panel component to use div instead of BoxWithFallback and removes BoxProps dependency |
| export const UnderlineItem = React.forwardRef((props, ref) => { | ||
| const {as: Component = 'a', children, counter, icon: Icon, iconsVisible, loadingCounters, className, ...rest} = props |
There was a problem hiding this comment.
[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.
| ) : null} | ||
| </Component> | ||
| ) | ||
| }) as PolymorphicForwardRefComponent<ElementType, UnderlineItemProps<ElementType>> |
There was a problem hiding this comment.
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.
| }) as PolymorphicForwardRefComponent<ElementType, UnderlineItemProps<ElementType>> | |
| }) as PolymorphicForwardRefComponent<keyof JSX.IntrinsicElements | React.ComponentType<any>, UnderlineItemProps<any>> |
Copilot uses AI. Check for mistakes.
github-actions
bot
requested a deployment
to
storybook-preview-6874
|
I understand this PR deals somewhat with |
| 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> |
|
Let's make sure to run integration checks on it before merging 🙏 |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/2970 |
|
🟢 ci completed with status |
github-actions
bot
added
integration-tests: passing
Closes ##6804
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist