-
Notifications
You must be signed in to change notification settings - Fork 631
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.
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
base: main
Are you sure you want to change the base?
Changes from 9 commits
9548fcc
7e4132e
54c09fb
64d1fc9
82a68db
da12e6e
a868c4d
842347d
aec771a
b4bd28b
891bcf8
6587d22
24c9bdf
163e7bd
4962e4f
8850160
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@primer/react": major | ||
--- | ||
|
||
Remove sx from UnderlinePanels |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,36 +1,37 @@ | ||||||
// Used for UnderlineNav and UnderlinePanels components | ||||||
|
||||||
import type React from 'react' | ||||||
import {forwardRef, type FC, type PropsWithChildren} from 'react' | ||||||
import React from 'react' | ||||||
import {type ForwardedRef, 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 | ||||||
|
||||||
type UnderlineWrapperProps = { | ||||||
type UnderlineWrapperProps<As extends React.ElementType> = { | ||||||
slot?: string | ||||||
as?: React.ElementType | ||||||
as?: As | ||||||
className?: string | ||||||
ref?: React.Ref<unknown> | ||||||
} & SxProp | ||||||
ref?: React.Ref<HTMLElement> | ||||||
} | ||||||
|
||||||
export const UnderlineWrapper = forwardRef( | ||||||
({children, className, ...rest}: PropsWithChildren<UnderlineWrapperProps>, forwardedRef) => { | ||||||
return ( | ||||||
<BoxWithFallback className={clsx(classes.UnderlineWrapper, className)} ref={forwardedRef} {...rest}> | ||||||
{children} | ||||||
</BoxWithFallback> | ||||||
) | ||||||
}, | ||||||
) | ||||||
export const UnderlineWrapper = forwardRef((props, ref) => { | ||||||
const {children, className, as: Component = 'nav', ...rest} = props | ||||||
return ( | ||||||
<Component | ||||||
className={clsx(classes.UnderlineWrapper, className)} | ||||||
ref={ref as ForwardedRef<HTMLDivElement>} | ||||||
{...rest} | ||||||
> | ||||||
{children} | ||||||
</Component> | ||||||
) | ||||||
}) as PolymorphicForwardRefComponent<ElementType, UnderlineWrapperProps<ElementType>> | ||||||
|
||||||
export const UnderlineItemList = forwardRef(({children, ...rest}: PropsWithChildren, forwardedRef) => { | ||||||
return ( | ||||||
|
@@ -44,51 +45,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> | ||||||
|
||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
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>> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type assertion uses
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,6 @@ import { | |
Token, | ||
Tooltip, | ||
Truncate, | ||
UnderlineNav, | ||
} from '../' | ||
|
||
describe('@primer/react', () => { | ||
|
@@ -422,22 +421,4 @@ describe('@primer/react', () => { | |
render(<Truncate data-testid="component" sx={{background: 'red'}} title="test" />) | ||
expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') | ||
}) | ||
|
||
test('UnderlineNav supports `sx` prop', () => { | ||
render( | ||
<UnderlineNav aria-label="navigation" data-testid="component" sx={{background: 'red'}}> | ||
<UnderlineNav.Item>test</UnderlineNav.Item> | ||
</UnderlineNav>, | ||
) | ||
expect(window.getComputedStyle(screen.getByLabelText('navigation')).backgroundColor).toBe('rgb(255, 0, 0)') | ||
}) | ||
|
||
test('UnderlineNav.Item supports `sx` prop', () => { | ||
render( | ||
<UnderlineNav.Item data-testid="component" sx={{background: 'red'}}> | ||
test | ||
</UnderlineNav.Item>, | ||
) | ||
expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') | ||
}) | ||
Comment on lines
476
to
492
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these test for Instead, we can wrap it as in this guide in For Curious what you think 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is true! nice catch, I totally missed this 😳 |
||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😵💫