-
Notifications
You must be signed in to change notification settings - Fork 646
feat: add leadingVisual to UnderlineNav.Item #7200
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
Conversation
🦋 Changeset detectedLatest commit: 47ebc3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
👋 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 |
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.
Pull Request Overview
This PR adds support for the leadingVisual prop to UnderlineNav.Item while deprecating the existing icon prop, aligning the component with Primer's design system conventions used in other components.
Key Changes:
- Introduces
leadingVisualprop acceptingReact.ReactElementas the new API for rendering visuals before item text - Deprecates the
iconprop with a clear migration path toleadingVisual - Updates all internal usages across tests and stories from function components to JSX elements
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/UnderlineNav/UnderlineNavItem.tsx | Adds leadingVisual prop, marks icon as deprecated, implements backwards-compatible fallback logic |
| packages/react/src/UnderlineNav/UnderlineNav.test.tsx | Updates test data types from FC<IconProps> to React.ReactElement, adds backwards compatibility test for deprecated icon prop |
| packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx | Migrates story examples from icon to leadingVisual, updates data types to use React elements |
| packages/react/src/UnderlineNav/UnderlineNav.examples.stories.tsx | Migrates examples from icon to leadingVisual, removes unused Octicon wrapper, updates data types |
| packages/react/src/UnderlineNav/UnderlineNav.docs.json | Adds documentation for leadingVisual prop and marks icon as deprecated |
| .changeset/old-yaks-win.md | Documents the minor version change for the new feature |
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.
Makes sense!
completely optional, i know you have other work as well: Would love to see a eslint rule to go with it!
|
@siddharthkp good point, I made an issue for this over at: https://github.com/github/primer/issues/6144 to see if we could add support for repurposing the |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/7243 |
|
🟢 ci completed with status |
…visual-to-underline-nav
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
Closes https://github.com/github/primer/issues/5289
Add support for
leadingVisualtoUnderlineNav.Itemand update existing references foricontoleadingVisual.Changelog
New
leadingVisualtoUnderlineNav.Itemwith theReact.ReactElementtypeChanged
iconprop in favor ofleadingVisualRemoved
Rollout strategy