-
Notifications
You must be signed in to change notification settings - Fork 639
feat(ActionList): Combine Item and LinkItem into unified Item component #6999
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?
Conversation
…ories and checked DOM rendering
…-and-item-in-actionlist
🦋 Changeset detectedLatest commit: 54ee453 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
…//github.com/primer/react into liuliu/merge-linkitem-and-item-in-actionlist
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 unifies ActionList.Item
and ActionList.LinkItem
into a single component by enabling ActionList.Item
to handle both regular items and link items based on the presence of link-related props (e.g., href
, target
, to
). Users can now create link items by simply adding anchor attributes to ActionList.Item
, eliminating the need to choose between two separate components. The existing ActionList.LinkItem
remains functional but is now deprecated.
Key changes include:
- Creation of
LinkItemContainerNoBox
component to handle link rendering within items - Extension of
ActionListItemProps
to includeLinkProps
for link-related attributes - Introduction of
INTERACTIVE_ELEMENT_PROPS
constant to properly distribute props between container and interactive elements - Deprecation of
ActionList.LinkItem
with backward compatibility maintained
Reviewed Changes
Copilot reviewed 20 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
packages/react/src/ActionList/shared.ts | Adds LinkProps type and INTERACTIVE_ELEMENT_PROPS constant for props distribution; extends ActionListItemProps to include link props |
packages/react/src/ActionList/Item.tsx | Implements unified item logic with link detection, adds LinkItemContainerNoBox component, and splits props between container and interactive elements |
packages/react/src/ActionList/LinkItem.tsx | Refactors to import LinkProps from shared.ts and removes duplicate type definition |
packages/react/src/ActionList/index.ts | Updates LinkItem export with deprecation notice |
packages/react/src/ActionList/ActionList.docs.json | Adds documentation for new link-related props on ActionList.Item |
Multiple story and test files | Migrates ActionList.LinkItem usage to ActionList.Item with href prop throughout examples, tests, and stories |
packages/react/src/NavList/NavList.tsx | Updates NavList to use ActionList.Item with as="a" instead of ActionList.LinkItem |
packages/react/src/UnderlineNav/UnderlineNav.tsx | Migrates from ActionList.LinkItem to ActionList.Item with ESLint disable comment for onClick |
.changeset/shiny-pianos-wash.md | Adds changeset documenting the major breaking change |
...props, | ||
...menuItemProps, | ||
inactiveText, | ||
userOnClick: interactiveProps.onClick as ((event: React.MouseEvent<HTMLAnchorElement>) => void) | undefined, |
Copilot
AI
Oct 16, 2025
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.
The type assertion could be replaced with a safer type guard or validation. Consider checking if interactiveProps.onClick
is a function before passing it as userOnClick
to avoid potential runtime issues if the value is not actually a function.
userOnClick: interactiveProps.onClick as ((event: React.MouseEvent<HTMLAnchorElement>) => void) | undefined, | |
userOnClick: typeof interactiveProps.onClick === 'function' ? interactiveProps.onClick : undefined, |
Copilot uses AI. Check for mistakes.
const isLinkItem = Boolean( | ||
props.href || | ||
props.download || | ||
props.target || | ||
props.rel || | ||
props.hrefLang || | ||
props.to || | ||
(typeof props.as === 'string' && props.as.toLowerCase() === 'a') || | ||
role === 'link', | ||
) |
Copilot
AI
Oct 16, 2025
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.
[nitpick] The link detection logic could be extracted into a utility function (e.g., isLinkItemProps
) to improve testability and reusability. This would make it easier to maintain the logic if the criteria for detecting link items changes in the future.
Copilot uses AI. Check for mistakes.
'@primer/react': major | ||
--- | ||
|
||
ActionList.Item now supports link functionality through href and other anchor attributes, deprecating the separate ActionList.LinkItem component. |
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.
Hi!
What does the deprecation show up as? Does it show up as an error in CI for github-ui or only a soft suggestion in vscode?
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.
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/5128 |
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.
Big PR with tricky changes, well done!
Left some questions to understand the changes better
<ActionList.LinkItem | ||
<ActionList.Item | ||
ref={ref} | ||
as="a" |
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.
This has as="a"
but does not have an href
or to
, should we throw a typescript error when that happens?
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.
I think it makes sense to validate for href
or to
when as="a"
is used
I added validation in Item.tsx
:
if (typeof props.as === 'string' && props.as.toLowerCase() === 'a' && !inactiveText) {
invariant(
props.href || props.to,
'ActionList.Item with as="a" must have an href or to prop for proper link semantics and accessibility.',
)
}
However, this will break existing usages in github-ui where NavList.Item
is used without href
(like this example).
<ActionList.Item | ||
key={menuItemChildren} | ||
style={menuItemStyles} | ||
/* eslint-disable-next-line primer-react/prefer-action-list-item-onselect */ |
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.
(non blocker) worth asking @pksjce or @broccolinisoup if there is a reason to prefer onClick over onSelect or if we can use onSelect here
Item with inline description | ||
<ActionList.Description variant="block">Block description</ActionList.Description> | ||
</ActionList.LinkItem> | ||
</ActionList.Item> |
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.
I think we should keep one story for LinkItem in ActionList.dev.stories so that we don't accidentally break it before it's time to remove ActionList.LinkItem.
props.hrefLang || | ||
props.to || | ||
(typeof props.as === 'string' && props.as.toLowerCase() === 'a') || | ||
role === 'link', |
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.
can we reduce this list to the main things that make it anchor tag
For example, having hrefLang without href does not make it a link. I imagine the list is just href, as and to.
* - Test identifiers | ||
*/ | ||
export const INTERACTIVE_ELEMENT_PROPS = [ | ||
// Link-specific props from LinkProps |
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.
How can we make sure this list is exhaustive?
'aria-keyshortcuts', | ||
// Styling props (should be on the interactive element) | ||
'style', | ||
'sx', |
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.
just checking, do we still want sx here?
const interactiveProps: Record<string, unknown> = {} | ||
|
||
for (const [key, value] of Object.entries(props)) { | ||
if (interactivePropsSet.has(key)) { |
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.
Sorry, just curious, is there a benefit of converting the array into a set to use Set.has instead of Array.includes? Took me a second to follow the trail 😅
// 3. default - regular button/div behavior | ||
|
||
// Create a Set for lookup of interactive-only props | ||
const interactivePropsSet = new Set<string>(INTERACTIVE_ELEMENT_PROPS) |
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.
This is a longer list than what we checked before, are there any props that were earlier going to the <li>
that will now end up on <a>
? Especially in instances where ActionList.Item was used to act like a Link
For example: ActionList.Item as="a" href=""
or ActionList.Item onClick={...}
} | ||
: isLinkItem | ||
? { | ||
...props, |
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.
just checking, do we want to spread all the props here? Or only some of the props?
...props, | ||
...menuItemProps, | ||
inactiveText, | ||
userOnClick: interactiveProps.onClick as ((event: React.MouseEvent<HTMLAnchorElement>) => void) | undefined, |
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.
Are we also using the other props in interactiveProps somewhere else? I could only spot onClick?
Heads up! There seem to be more tests that are failing in the integration PR that the comment does not know about: https://github.com/github/github-ui/pull/5128. I have not checked if they are related to changes in this PR, may or may not be. |
…-and-item-in-actionlist
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
🔴 ci completed with status |
Summary
Unifies
ActionList.Item
andActionList.LinkItem
into a singleActionList.Item
component that handles both regular items and link items. Users can create link items by simply addinghref
or other anchor attributes toActionList.Item
, without needing to choose between two separate components.All existing
ActionList.LinkItem
usage continues to work unchanged.Closes https://github.com/github/primer/issues/5956
Implementation
Link Container
Created
LinkItemContainerNoBox
following the pattern ofButtonItemContainerNoBox
/DivItemContainerNoBox
, used asItemWrapper
when Item should be rendered as a link .The structure is similar to
_PrivateItemWrapper
used in theLinkItem
, but with some props changes : passinguserOnClick
andinactiveText
which are no longer in scope.Props Merging
The challenge is
ActionListItemProps
now includes both previousActionListItemProps
andLinkProps
.To avoid breaking current usage and support migration from
ActionList.LinkItem
toActionList.Item
with just the component name change (no props change), definedINTERACTIVE_ELEMENT_PROPS
const inshared.ts
. This const is used to strip LinkItem-specific props and only passcontainerOnlyProps
to container, avoiding props likehref
showing up in<li>
.Props are split into
interactiveProps
andcontainerOnlyProps
.Migration
Change component name, props stay the same:
Breaking Changes
ActionList.LinkItem
deprecated (still works but will show deprecation warnings)ActionListItemProps
now includes link props. And removed unusedtype
prop fromLinkProps
, it caused type conflicts(https://github.com/github/github-ui/blob/b6bf6d94ab0a339243ebdcad5e6eeefce2b93f48/packages/copilot-chat/components/autocomplete/FileSuggestion.tsx#L8) when extending the interface.Notes
The ESLint rule
primer-react/prefer-action-list-item-onselect
will need to be updated to allowonClick
when link props are present.Are these conditions sufficient to detect link items? Should any other props trigger link behavior?
Rollout strategy