ActionList: Add more checks for ActionList.Item when using button semantics
#4876
Conversation
🦋 Changeset detectedLatest commit: 84aaff8 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 |
size-limit report 📦
|
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/344691 |
| @@ -24,6 +24,14 @@ import VisuallyHidden from '../_VisuallyHidden' | |||
|
|
|||
| const LiBox = styled.li<SxProp>(sx) | |||
|
|
|||
| const ButtonItemContainer = React.forwardRef(({as: Component = 'button', children, styles, ...props}, forwardedRef) => { | |||
| return ( | |||
| <Box as={Component as React.ElementType} ref={forwardedRef} sx={styles} {...props}> | |||
There was a problem hiding this comment.
just checking, do we need to merge styles like ButtonItemWrapper or is that not needed anymore?
There was a problem hiding this comment.
I saw we were already handling this on line #329, so now we pass the same logic to ButtonItemContainer, but under the styles prop instead of handling it inside of the component.
| const listItemSemantics = | ||
| role === 'option' || role === 'menuitem' || role === 'menuitemradio' || role === 'menuitemcheckbox' | ||
|
|
||
| const listRoleTypes = ['listbox', 'menu', 'list'] |
There was a problem hiding this comment.
Oooh, list is a new addition here. Can you say a bit more about why list was added here and what the remaining cases when we want buttonSemantics to be used?
There was a problem hiding this comment.
We don't have any live cases of role="list" with ActionList, so this was mainly just an escape hatch. There are odd cases out there where the new semantics might not be as ideal, one I can think of is when ActionList is utilized as a way to load in items (e.g. loading items story). The only issue is, they probably shouldn't be interactive if they're role="list".
Not really beholden to having it here, but thought it could be useful as a escape hatch for components that still need the old semantics, at least temporarily.
Co-authored-by: Siddharth Kshetrapal <[email protected]>
|
👋🏻 Bumping this for priority. The associated issue to this PR https://github.com/github/primer/issues/3722 was listed as a short term repair item, which has an SLA for repair of 60 days and we are now 51 days overdue. Sid is out of office right now so we'll need to identify another reviewer. @camertron as the other engineer tagged for review, could you please add this to your queue to get reviewed this week or very early next week? |
|
👋 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
bot
added
the
integration-tests: recommended
Part of https://github.com/github/primer/issues/3722
Changelog
Changed
ActionList.ItemRollout strategy
Testing & Reviewing
Merge checklist