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

1518 react treeview feedback from screen reader #2710

Merged
merged 15 commits into from
Jan 24, 2023

Conversation

green6erry
Copy link
Contributor

@green6erry green6erry commented Dec 19, 2022

Describe your changes here.

Closes 1518 as somewhat of a bandaid for the issue. The reason this is happening is from a larger issue with the api. For now, the aria message is now updating the status correctly based on which branch is expanded.

Screenshots

Please provide before/after screenshots for any visual changes
Before:
image
Directory with async items is expanded, but the status reads that that the last child, Nested Directory is empty. This is both not true, and irrelevant to expanding Directory with async items

After:
image
Now when Directory with async items is expanded, the status accurately reads that it has loaded.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@green6erry green6erry requested review from a team and mperrotti December 19, 2022 16:14
@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2022

🦋 Changeset detected

Latest commit: 84825f0

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 Dec 19, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 89.01 KB (0%)
dist/browser.umd.js 89.62 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-2710 December 19, 2022 16:20 Inactive
@green6erry green6erry temporarily deployed to github-pages December 19, 2022 16:22 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 December 19, 2022 16:23 Inactive
@green6erry green6erry self-assigned this Dec 19, 2022
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 December 19, 2022 17:01 Inactive
@green6erry green6erry temporarily deployed to github-pages December 19, 2022 17:05 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 December 19, 2022 17:05 Inactive
@colebemis
Copy link
Contributor

The reason this is happening is from a larger issue with the API.

Could you expand on this? What do you think the root cause of this issue is?

src/TreeView/TreeView.tsx Outdated Show resolved Hide resolved
src/TreeView/TreeView.tsx Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 December 29, 2022 17:04 Inactive
@green6erry green6erry temporarily deployed to github-pages December 29, 2022 17:05 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 December 29, 2022 17:06 Inactive
@green6erry green6erry temporarily deployed to github-pages January 3, 2023 15:49 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 January 3, 2023 15:49 Inactive
@green6erry green6erry temporarily deployed to github-pages January 10, 2023 19:26 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 January 10, 2023 19:26 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Thanks for walking me through these changes, @green6erry! Looks good to me. Just left a small suggestion about adding a code comment to explain isPending.

@green6erry green6erry temporarily deployed to github-pages January 23, 2023 16:33 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 January 23, 2023 16:34 Inactive
@green6erry green6erry force-pushed the 1518-react-treeview-feedback-from-screen-reader branch from 7f02e48 to e22e310 Compare January 23, 2023 21:55
@green6erry green6erry temporarily deployed to github-pages January 23, 2023 22:03 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 January 23, 2023 22:04 Inactive
Co-authored-by: Cole Bemis <colebemis@github.com>
@green6erry green6erry temporarily deployed to github-pages January 24, 2023 17:39 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 January 24, 2023 17:40 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 January 24, 2023 17:49 Inactive
@green6erry green6erry temporarily deployed to github-pages January 24, 2023 17:49 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2710 January 24, 2023 17:50 Inactive
@green6erry green6erry enabled auto-merge (squash) January 24, 2023 17:54
@green6erry green6erry temporarily deployed to github-pages January 24, 2023 17:57 — with GitHub Actions Inactive
@green6erry green6erry merged commit ac0abd1 into main Jan 24, 2023
@green6erry green6erry deleted the 1518-react-treeview-feedback-from-screen-reader branch January 24, 2023 17:59
@primer-css primer-css mentioned this pull request Jan 24, 2023
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.

2 participants