-
Notifications
You must be signed in to change notification settings - Fork 356
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
docs(TreeView): add example with unique icon per item #11377
docs(TreeView): add example with unique icon per item #11377
Conversation
Preview: https://patternfly-react-pr-11377.surge.sh A11y report: https://patternfly-react-pr-11377-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.
LGTM 👍
name: 'Github accounts', | ||
id: 'iconPerItem-Github', | ||
icon: <GithubIcon />, | ||
expandedIcon: <GithubIcon />, |
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.
Since these icons are the same, we could omit the expandedIcon prop and the default icon
will be retained. Having this in the example may lead consumers to think that you have to pass both props even if the icon doesn't change.
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.
Good point! I initially thought it might be a good idea to show customers that "hey, you can also customize the expanded icon prop if you want". But you are right that it is rather misleading.
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.
Ah yeah that might be good, possibly one of the parent items retains that folder icon from the previous example to show the default and expanded icon. If you would want to do something like that that'd be good to me, but as-is this looks good
Closes #10236