feat(ActionList): Combine Item and LinkItem into unified Item component#6999
feat(ActionList): Combine Item and LinkItem into unified Item component#6999liuliu-dev wants to merge 27 commits intomainfrom
Conversation
…ories and checked DOM rendering
…-and-item-in-actionlist
🦋 Changeset detectedLatest commit: c6f608b 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
|
👋 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! |
| <ActionList.LinkItem | ||
| <ActionList.Item | ||
| ref={ref} | ||
| as="a" |
There was a problem hiding this comment.
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.
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.
(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.
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.
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.
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.
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.
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.
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.
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.
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! |
|
👋 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
|
👋 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, 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 |
|
😢 Hi from github/github-ui. The integration workflow has failed: https://github.com/github/github-ui/actions/runs/18978867808 |
Summary
Unifies
ActionList.ItemandActionList.LinkIteminto a singleActionList.Itemcomponent that handles both regular items and link items. Users can create link items by simply addinghrefor other anchor attributes toActionList.Item, without needing to choose between two separate components.All existing
ActionList.LinkItemusage continues to work unchanged.Closes https://github.com/github/primer/issues/5956
Implementation
Link Container
Created
LinkItemContainerNoBoxfollowing the pattern ofButtonItemContainerNoBox/DivItemContainerNoBox, used asItemWrapperwhen Item should be rendered as a link .The structure is similar to
_PrivateItemWrapperused in theLinkItem, but with some props changes : passinguserOnClickandinactiveTextwhich are no longer in scope.Props Merging
The challenge is
ActionListItemPropsnow includes both previousActionListItemPropsandLinkProps.To avoid breaking current usage and support migration from
ActionList.LinkItemtoActionList.Itemwith just the component name change (no props change), definedINTERACTIVE_ELEMENT_PROPSconst inshared.ts. This const is used to strip LinkItem-specific props and only passcontainerOnlyPropsto container, avoiding props likehrefshowing up in<li>.Props are split into
interactivePropsandcontainerOnlyProps.Migration
Change component name, props stay the same:
Breaking Changes
ActionList.LinkItemdeprecated (still works but will show deprecation warnings)ActionListItemPropsnow includes link props. And removed unusedtypeprop 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-onselectwill need to be updated to allowonClickwhen link props are present.Are these conditions sufficient to detect link items? Should any other props trigger link behavior?
Rollout strategy