Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions .changeset/underline-nav-icon-breakpoint.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

UnderlineNav: Keep item icons visible by default except below the xsmall breakpoint

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

when was the original default added? depending on the timeline this has been around, might be worth considering whether this is a major change👀

patch: we accidentally added the wrong default value in a recent release and are correcting it
major: we're changing the behavior of this component by changing this default

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this default was added recently in #8133 a month ago and first shipped in 38.32.0 three weeks ago. Since this is a recent change, would a patch be reasonable? 👀

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok yeah SGTM!

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

double checking that this diff is correct ? seems some shading around the "more" button is gone

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch! I reverted this snapshot change.

7 changes: 5 additions & 2 deletions e2e/components/UnderlineNav.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,11 @@ test.describe('UnderlineNav', () => {
await page.getByRole('button', {name: 'More items'}).click()
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot()

await page.getByRole('menuitem', {name: 'Settings (10)'}).click()
expect(await page.screenshot()).toMatchSnapshot()
const settingsMenuItem = page.getByRole('menuitem', {name: 'Settings (10)'})
await settingsMenuItem.click()
await settingsMenuItem.waitFor({state: 'hidden'})
await page.getByRole('button', {name: 'More items, including current item'}).waitFor()
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot()
})

test('Hide icons when there is not enough space to display all list items @vrt', async ({page}) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const items: {navigation: string; icon: React.ReactElement; counter?: number | s
export const OverflowTemplate = ({initialSelectedIndex = 1}: {initialSelectedIndex?: number}) => {
const [selectedIndex, setSelectedIndex] = React.useState<number | null>(initialSelectedIndex)
return (
<UnderlineNav aria-label="Repository">
<UnderlineNav aria-label="Repository" hideIconsBreakpoint="medium">
{items.map((item, index) => (
<UnderlineNav.Item
key={item.navigation}
Expand Down
23 changes: 23 additions & 0 deletions packages/react/src/UnderlineNav/UnderlineNav.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,29 @@ describe('UnderlineNav', () => {
expect(list.getElementsByTagName('svg').length).toEqual(7)
})

it('hides icons below the xsmall breakpoint by default', () => {
render(<ResponsiveUnderlineNav />)
expect(screen.getByRole('navigation')).toHaveAttribute('data-hide-icons-breakpoint', 'xsmall')
})

it('supports customizing when icons are hidden', () => {
render(
<UnderlineNav aria-label="Repository" hideIconsBreakpoint="medium">
<UnderlineNav.Item>Code</UnderlineNav.Item>
</UnderlineNav>,
)
expect(screen.getByRole('navigation')).toHaveAttribute('data-hide-icons-breakpoint', 'medium')
})

it('supports always showing icons', () => {
render(
<UnderlineNav aria-label="Repository" hideIconsBreakpoint={null}>
<UnderlineNav.Item>Code</UnderlineNav.Item>
</UnderlineNav>,
)
expect(screen.getByRole('navigation')).not.toHaveAttribute('data-hide-icons-breakpoint')
})

it('fires onSelect on click', async () => {
const onSelect = vi.fn()
const {getByRole} = render(
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/UnderlineNav/UnderlineNav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export type UnderlineNavProps = {
variant?: 'inset' | 'flush'
/**
* Customize when the icons are hidden to save space. Set to `null` to always show icons.
* Defaults to `xxlarge`; if you know there won't be very many tabs you might set this to a smaller value.
* Defaults to `xsmall`.
*/
hideIconsBreakpoint?: 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge' | null
}
Expand All @@ -47,7 +47,7 @@ export const UnderlineNav = forwardRef(
variant = 'inset',
className,
children,
hideIconsBreakpoint = 'xxlarge',
hideIconsBreakpoint = 'xsmall',
}: UnderlineNavProps,
forwardedRef,
) => {
Expand Down
Loading