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

feat(TreeView): add support for empty parent nodes #2478

Merged
merged 7 commits into from
Oct 24, 2022

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Oct 24, 2022

This PR updates TreeView.Item and TreeView.SubTree based on the feedback over in: https://github.com/github/primer/pull/1389#discussion_r998754809

Changelog

New

  • Add EmptyDirectory story for TreeView

Changed

  • Update TreeView.Item and TreeView.SubTree to coordinate whether or not aria-expanded should be set in sync and async contexts
  • Update TreeView.Item to announce when a TreeView.SubTree has no content when used in an async context
  • Update TreeView.Item to shift conditional rendering of TreeView.SubTree into TreeView.SubTree
  • Update TreeView.test.tsx to remove it.only and replace timers with jest.useFakeTimers

Removed

@changeset-bot
Copy link

changeset-bot bot commented Oct 24, 2022

🦋 Changeset detected

Latest commit: 19a9f0a

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

@joshblack joshblack marked this pull request as ready for review October 24, 2022 17:35
@joshblack joshblack requested review from a team, mperrotti and colebemis October 24, 2022 17:35
@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 77.99 KB (0%)
dist/browser.umd.js 78.64 KB (0%)

@joshblack joshblack temporarily deployed to github-pages October 24, 2022 17:40 Inactive
@joshblack joshblack temporarily deployed to github-pages October 24, 2022 19:53 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.

Looks great! Could you add a couple of tests to make sure the aria-expanded attribute is behaving as expected in the sync and async cases?

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 tackling this tricky implementation 💖

Should be ready to merge after fixing the lint error.

@joshblack joshblack enabled auto-merge (squash) October 24, 2022 21:03
@joshblack joshblack temporarily deployed to github-pages October 24, 2022 21:06 Inactive
@joshblack joshblack merged commit 7cbb5c9 into main Oct 24, 2022
@joshblack joshblack deleted the feat/add-empty-load-case branch October 24, 2022 21:10
@primer-css primer-css mentioned this pull request Oct 24, 2022
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