Skip to content
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

fix(ActionList): place id on item with role #3691

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Aug 30, 2023

Context: https://github.slack.com/archives/C01L618AEP9/p1693422238328979

The id on ActionList.Item should be placed on the item with the corresponding ARIA role so that a consumer can use that value to manage attributes like aria-activedescendant appropriately.

Warning
This change could be considered a breaking change if we consider the id placement as something that should be stable across minor versions.

@joshblack joshblack requested review from a team and mperrotti August 30, 2023 19:26
@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2023

🦋 Changeset detected

Latest commit: 0a60663

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

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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
Contributor

github-actions bot commented Aug 30, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 104.7 KB (+0.01% 🔺)
dist/browser.umd.js 105.28 KB (+0.01% 🔺)

@joshblack joshblack temporarily deployed to github-pages August 30, 2023 19:32 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3691 August 30, 2023 19:32 Inactive
Copy link
Contributor

@mperrotti mperrotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI should pass after updating snapshots.

@joshblack joshblack temporarily deployed to github-pages September 6, 2023 20:28 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3691 September 6, 2023 20:29 Inactive
@joshblack
Copy link
Member Author

@dusave checking upstream, it doesn't seem like this change introduces a regression so it could be safe to land. Wanted to double-check with you to see how you felt about this change.

I'll also ask the team to see if we're okay with this kind of change in a current minor release 👀

@joshblack joshblack temporarily deployed to github-pages September 7, 2023 16:10 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3691 September 7, 2023 16:10 Inactive
Copy link
Contributor

@dusave dusave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Awesome!

@github-actions github-actions bot temporarily deployed to storybook-preview-3691 September 13, 2023 15:30 Inactive
@joshblack joshblack added this pull request to the merge queue Oct 10, 2023
Merged via the queue into main with commit f4648b1 Oct 10, 2023
30 checks passed
@joshblack joshblack deleted the fix/update-action-list branch October 10, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants