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 |
adamviktora
requested review from
a team,
rebeccaalpert and
wise-king-sullyman
and removed request for
a team
rebeccaalpert
left a comment
rebeccaalpert
left a comment
There was a problem hiding this comment.
Code looks good. My only suggestion would be that we add a test for this if you have time.
adamviktora
force-pushed
the
treeViewListItem-custom-badge
branch
from
f6a83e8 to
d29a114
Compare
rebeccaalpert
left a comment
rebeccaalpert
left a comment
There was a problem hiding this comment.
Thank you for the test 🙌
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
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)
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
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. |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
@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! 🎉 |
5 participants
Closes #11845