Skip to content

feat(ActionList): Combine Item and LinkItem into unified Item component#6999

Closed
liuliu-dev wants to merge 27 commits into
mainfrom
liuliu/merge-linkitem-and-item-in-actionlist
Closed

feat(ActionList): Combine Item and LinkItem into unified Item component#6999
liuliu-dev wants to merge 27 commits into
mainfrom
liuliu/merge-linkitem-and-item-in-actionlist

Conversation

@liuliu-dev

@liuliu-dev liuliu-dev commented Oct 13, 2025

Copy link
Copy Markdown
Contributor

Summary

Unifies ActionList.Item and ActionList.LinkItem into a single ActionList.Item component that handles both regular items and link items. Users can create link items by simply adding href or other anchor attributes to ActionList.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 of ButtonItemContainerNoBox /DivItemContainerNoBox, used as ItemWrapper when Item should be rendered as a link .

The structure is similar to _PrivateItemWrapper used in the LinkItem, but with some props changes : passing userOnClick and inactiveText which are no longer in scope.

Props Merging
The challenge is ActionListItemProps now includes both previous ActionListItemProps and LinkProps.

To avoid breaking current usage and support migration from ActionList.LinkItem to ActionList.Item with just the component name change (no props change), defined INTERACTIVE_ELEMENT_PROPS const in shared.ts. This const is used to strip LinkItem-specific props and only pass containerOnlyProps to container, avoiding props like href showing up in <li>.

Props are split into interactiveProps and containerOnlyProps.

Migration
Change component name, props stay the same:

// Before
<ActionList.LinkItem href="/path">Link</ActionList.LinkItem>

// After
<ActionList.Item href="/path">Link</ActionList.Item>

Breaking Changes

Notes

  • The ESLint rule primer-react/prefer-action-list-item-onselect will need to be updated to allow onClick when link props are present.

  • Are these conditions sufficient to detect link items? Should any other props trigger link behavior?

  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',
  )

Rollout strategy

  • Major release; if selected, include a written rollout or migration plan

@changeset-bot

changeset-bot Bot commented Oct 13, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c6f608b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@primer/react Major
@primer/styled-react Major

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

@github-actions

Copy link
Copy Markdown
Contributor

👋 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-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 Oct 13, 2025
@github-actions github-actions Bot temporarily deployed to storybook-preview-6999 October 13, 2025 22:35 Inactive
@liuliu-dev liuliu-dev added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Oct 13, 2025
@github-actions github-actions Bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Oct 13, 2025
@github-actions github-actions Bot temporarily deployed to storybook-preview-6999 October 13, 2025 23:06 Inactive
@liuliu-dev liuliu-dev added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Oct 14, 2025
@github-actions github-actions Bot requested a deployment to storybook-preview-6999 October 14, 2025 23:00 Abandoned
@github-actions github-actions Bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Oct 14, 2025
@github-actions github-actions Bot temporarily deployed to storybook-preview-6999 October 14, 2025 23:08 Inactive
@github-actions github-actions Bot temporarily deployed to storybook-preview-6999 October 14, 2025 23:16 Inactive
@github-actions github-actions Bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 14, 2025
@github-actions github-actions Bot added integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 20, 2025
@github-actions

Copy link
Copy Markdown
Contributor

👋 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!

@github-actions github-actions Bot temporarily deployed to storybook-preview-6999 October 20, 2025 23:38 Inactive
@github-actions github-actions Bot removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Oct 20, 2025

@siddharthkp siddharthkp left a comment

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.

Big PR with tricky changes, well done!

Left some questions to understand the changes better

Comment thread packages/react/src/NavList/NavList.tsx Outdated
<ActionList.LinkItem
<ActionList.Item
ref={ref}
as="a"

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.

This has as="a" but does not have an href or to, should we throw a typescript error when that happens?

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.

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 */

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.

(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>

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.

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.

Comment thread packages/react/src/ActionList/Item.tsx Outdated
props.hrefLang ||
props.to ||
(typeof props.as === 'string' && props.as.toLowerCase() === 'a') ||
role === 'link',

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.

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.

Comment thread packages/react/src/ActionList/shared.ts Outdated
* - Test identifiers
*/
export const INTERACTIVE_ELEMENT_PROPS = [
// Link-specific props from LinkProps

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.

How can we make sure this list is exhaustive?

Comment thread packages/react/src/ActionList/shared.ts Outdated
'aria-keyshortcuts',
// Styling props (should be on the interactive element)
'style',
'sx',

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.

just checking, do we still want sx here?

Comment thread packages/react/src/ActionList/Item.tsx Outdated
const interactiveProps: Record<string, unknown> = {}

for (const [key, value] of Object.entries(props)) {
if (interactivePropsSet.has(key)) {

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.

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 😅

Comment thread packages/react/src/ActionList/Item.tsx Outdated
// 3. default - regular button/div behavior

// Create a Set for lookup of interactive-only props
const interactivePropsSet = new Set<string>(INTERACTIVE_ELEMENT_PROPS)

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.

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,

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.

just checking, do we want to spread all the props here? Or only some of the props?

Comment thread packages/react/src/ActionList/Item.tsx Outdated
...props,
...menuItemProps,
inactiveText,
userOnClick: interactiveProps.onClick as ((event: React.MouseEvent<HTMLAnchorElement>) => void) | undefined,

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.

Are we also using the other props in interactiveProps somewhere else? I could only spot onClick?

@siddharthkp

siddharthkp commented Oct 21, 2025

Copy link
Copy Markdown
Member

ci completed with status success.

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.

@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 Oct 21, 2025
@github-actions

Copy link
Copy Markdown
Contributor

👋 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!

@github-actions github-actions Bot temporarily deployed to storybook-preview-6999 October 21, 2025 19:03 Inactive
@github-actions github-actions Bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 21, 2025
@github-actions

Copy link
Copy Markdown
Contributor

👋 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!

3 similar comments
@github-actions

Copy link
Copy Markdown
Contributor

👋 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!

@github-actions

Copy link
Copy Markdown
Contributor

👋 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!

@github-actions

Copy link
Copy Markdown
Contributor

👋 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!

@primer-integration

Copy link
Copy Markdown

🔴 ci completed with status failure.

@primer-integration

Copy link
Copy Markdown

😢 Hi from github/github-ui. The integration workflow has failed: https://github.com/github/github-ui/actions/runs/18978867808

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

Labels

integration-tests: failing Changes in this PR cause breaking changes in gh/gh

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants