Skip to content

Conversation

@daantosaurus
Copy link

@daantosaurus daantosaurus commented Jan 15, 2026

Hello Primer reviewer 👋, this is my first time contributing to the repo, so please let me know if there's anything I can do better in terms of how I'm contributing.

This change stems from an initiative that involves adding popovers to the React global nav bar to prompt users to enable GHAS (example issue). I've already attempted to implement these popovers, but they result in a spacing issue stemming from UnderlineNav. See example below:

Screenshot 2026-01-14 at 11 43 48 PM

I believe that fixing this to allow better popover (or any other component that people can think of) support on the nav bar could open up the doors for other teams to add their own overlays to nav bar tabs and help them achieve their business needs!

See the linked issues above if needed for more context! I dive deeper into the root cause below

Changelog

The UnderlineNav.Item component accepts React.ReactNode as children but the internal implementation assumes children will always be a plain string. This causes layout issues when valid React elements are passed alongside the label text.

The data-content attribute which is used by CSS to reserve space for bold text in a nav item (in order to prevent layout shift) would break when the children included React elements, as they can't be properly serialized into a string. To fix this, I added a helper function that extracts only direct text strings from children in order to ignore React elements

Before, if a string and a ReactNode were passed in as children, data-content would evaluate to something like data-content="someString[object Object]". After this change, it would correctly evaluate to data-content="someString"

New

  • Added WithPopover story demonstrating Popover usage inside UNderlineNav.Item
image

Changed

  • UnderlineNav.Item now extracts text content for data-content when children contain React elements

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@changeset-bot
Copy link

changeset-bot bot commented Jan 15, 2026

⚠️ No Changeset found

Latest commit: 599cf5c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jan 15, 2026

Uh oh! @daantosaurus, at least one image you shared is missing helpful alt text. Check your pull request body to fix the following violations:

  • Images should have meaningful alternative text (alt text) at line 9
  • Images should have meaningful alternative text (alt text) at line 30

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jan 15, 2026
@github-actions
Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

@daantosaurus daantosaurus marked this pull request as ready for review January 15, 2026 20:27
@daantosaurus daantosaurus requested a review from a team as a code owner January 15, 2026 20:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a layout issue in UnderlineNav.Item when React elements (like Popovers) are included alongside text in children. The problem occurs with the data-content attribute used by CSS to reserve space for bold text (preventing layout shift). Previously, this attribute would incorrectly serialize React elements to [object Object], breaking the CSS mechanism.

Changes:

  • Added getTextContent helper function to extract only direct text content from children, ignoring nested React elements
  • Updated UnderlineItem to use the helper for the data-content attribute
  • Added comprehensive test coverage for the new functionality
  • Added Storybook story demonstrating Popover usage in UnderlineNav.Item

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
packages/react/src/internal/components/UnderlineTabbedInterface.tsx Adds getTextContent helper and updates UnderlineItem to extract text-only content for data-content attribute
packages/react/src/UnderlineNav/UnderlineNav.test.tsx Adds tests verifying text extraction works correctly with nested elements and plain strings
packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx Adds story demonstrating Popover usage inside UnderlineNav.Item

Comment on lines +19 to +28
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 ''
}
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.
<UnderlineNav.Item href="#security" leadingVisual={<ShieldLockIcon />} counter={12}>
Security
<Popover
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.
</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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants