-
Notifications
You must be signed in to change notification settings - Fork 368
fix(TreeViewListItem): customBadgeContent of number 0 will display #11848
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(TreeViewListItem): customBadgeContent of number 0 will display #11848
Conversation
Preview: https://patternfly-react-pr-11848.surge.sh A11y report: https://patternfly-react-pr-11848-a11y.surge.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. My only suggestion would be that we add a test for this if you have time.
f6a83e8
to
d29a114
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the test 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good. I think I want to just confirm the use-case for passing 0
to the customBadgeContent when children are also passed. Would the following screenshot be somewhat the intended result? Just using our custom badge example:
If so, that I think could be pretty confusing, but it depends on the context (i.e. if the "0" isn't conveying how many children there are, but in that case it'd be better to include some SR text in the badge as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Howdy stranger! 👋
Hey everybody! @thatblindgeye Adding a screen reader text is a good idea definitely. Our use case is that the badge indicates number of VirtualMachines under that tree item. But there can also be projects and folders as children: ![]() ![]() So e.g. if there are only projects with 0 VirtualMachines, the root should show 0 instead of number of projects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamviktora ah gotcha. SR text at the least would be helpful, though having the text visible might be better to make it clearer what the 0 means - but depending on the surrounding context or user-familiarity with the product maybe not necessary. This code change LGTM , though.
Your changes have been released in:
Thanks for your contribution! 🎉 |
Closes #11845