Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
import type {Meta} from '@storybook/react-vite'
import {UnderlineNav} from './index'
import {INITIAL_VIEWPORTS} from 'storybook/viewport'
import Popover from '../Popover'

const meta = {
title: 'Components/UnderlineNav/Features',
Expand Down Expand Up @@ -154,3 +155,35 @@ export const VariantFlush = () => {
</UnderlineNav>
)
}

export const WithPopover = () => {
return (
<UnderlineNav aria-label="Repository">
<UnderlineNav.Item href="#code" leadingVisual={<CodeIcon />}>
Code
</UnderlineNav.Item>
<UnderlineNav.Item href="#issues" leadingVisual={<IssueOpenedIcon />}>
Issues
</UnderlineNav.Item>
<UnderlineNav.Item href="#security" leadingVisual={<ShieldLockIcon />} counter={12}>
Security
<Popover
Copy link
Member

Choose a reason for hiding this comment

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

Ran into this on slack, so leaving some unsolicited notes:

There are a few things to consider here:

  1. Is the rendered DOM still accessible?
  2. When the item with popover is inside the overflow "More" menu, does it still position the popover correctly?
  3. Can the API for this be better? It feels a bit strange to put popover content inside the underline item. Is it already possible to put the Popover outside the UnderlineNav but still get it to position on the right anchor? Alternatively, should Popover take an anchorRef to attach itself to?

I'll leave these with @hectahertz :)

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah I see what you mean especially with point #3. I personally had a hard time trying to nicely position the popover on the NavItems while also rendering it outside of the UnderlineNav, so my mind is moving towards the idea of having Popover take an anchorRef or just using a custom AnchoredOverlay in github-ui where I add a custom caret to simulate the popover experience (since AnchoredOverlay already takes anchorRefs). Let me take this back to the drawing board and I'll keep in touch

In the meantime, do yall want me to continue with the fix in this PR? I can update the PR to keep the fix and I'll just remove the story I added. Otherwise I can just close it

Copy link
Author

@daantosaurus daantosaurus Jan 16, 2026

Choose a reason for hiding this comment

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

Oh, I should also ask yall for the recommended guidance to achieve this popover experience on the nav bar 😅. What do you think between the custom AchoredOverlay approach with a caret vs adding an anchorRef to Popover? Imo adding an anchorRef to popover makes sense but I'm far from the experienced one here

open={true}
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

The open prop should use the boolean value true directly instead of {true}. In JSX, boolean true values can be written simply as open without explicitly passing {true}.

Suggested change
open={true}
open

Copilot uses AI. Check for mistakes.
style={{
left: '50%',
transform: 'translateX(-50%)',
marginTop: 'var(--base-size-12)',
}}
>
<Popover.Content>Popover content</Popover.Content>
</Popover>
</UnderlineNav.Item>
<UnderlineNav.Item href="#insights" leadingVisual={<GraphIcon />}>
Insights
</UnderlineNav.Item>
<UnderlineNav.Item href="#settings" leadingVisual={<GearIcon />}>
Settings
</UnderlineNav.Item>
</UnderlineNav>
)
}
28 changes: 28 additions & 0 deletions packages/react/src/UnderlineNav/UnderlineNav.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,34 @@ describe('UnderlineNav', () => {
expect(screen.getByTestId('jsx-element')).toBeInTheDocument()
expect(screen.getByTestId('functional-component')).toBeInTheDocument()
})

it('extracts only direct text content for data-content attribute, ignoring nested elements', () => {
render(
<UnderlineNav aria-label="Test">
<UnderlineNav.Item>
Tab Label
<span style={{position: 'absolute'}}>Hidden element</span>
</UnderlineNav.Item>
</UnderlineNav>,
)

const item = screen.getByRole('link', {name: /Tab Label/})
const textSpan = item.querySelector('[data-component="text"]')
// data-content should only have the content of the Text and not the nested span
expect(textSpan).toHaveAttribute('data-content', 'Tab Label')
})

it('handles string children correctly for data-content attribute', () => {
render(
<UnderlineNav aria-label="Test">
<UnderlineNav.Item>Simple Text</UnderlineNav.Item>
</UnderlineNav>,
)

const item = screen.getByRole('link', {name: 'Simple Text'})
const textSpan = item.querySelector('[data-component="text"]')
expect(textSpan).toHaveAttribute('data-content', 'Simple Text')
})
})

describe('Keyboard Navigation', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ import {clsx} from 'clsx'
// 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

// Helper to extract direct text content from children for the data-content attribute.
// This is used by CSS to reserve space for bold text (preventing layout shift).
// Only extracts strings/numbers, not text from nested React elements (e.g., Popovers).
function getTextContent(children: React.ReactNode): string {
if (typeof children === 'string' || typeof children === 'number') {
return String(children)
}
if (Array.isArray(children)) {
return children.map(getTextContent).join('')
}
// Skip React elements - we only want direct text content, not text from nested components
return ''
}
Comment on lines +19 to +28
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

The getTextContent function lacks test coverage for important edge cases. While tests verify basic functionality with strings and nested elements, additional test cases should be added for: arrays of mixed content (strings and elements), numeric children, and empty arrays. Consider adding a test that verifies mixed arrays like ['Text', <span>element</span>, 123] correctly extracts 'Text123'.

Copilot uses AI. Check for mistakes.

type UnderlineWrapperProps<As extends React.ElementType> = {
slot?: string
as?: As
Expand Down Expand Up @@ -59,11 +73,12 @@ export type UnderlineItemProps<As extends React.ElementType> = {

export const UnderlineItem = React.forwardRef((props, ref) => {
const {as: Component = 'a', children, counter, icon: Icon, iconsVisible, loadingCounters, className, ...rest} = props
const textContent = getTextContent(children)
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}>
<span data-component="text" data-content={textContent || undefined}>
{children}
</span>
)}
Expand Down
Loading