-
Notifications
You must be signed in to change notification settings - Fork 350
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(Toolbar): deleteChip expects categoryKey as string #4553
fix(Toolbar): deleteChip expects categoryKey as string #4553
Conversation
@@ -67,21 +67,23 @@ export class ToolbarFilter extends React.Component<ToolbarFilterProps, ToolbarFi | |||
render() { | |||
const { children, chips, deleteChipGroup, deleteChip, categoryName, showToolbarItem, ...props } = this.props; | |||
const { isExpanded, chipGroupContentRef } = this.context; | |||
const categoryKey = typeof categoryName === 'string' ? categoryName : categoryName.key; |
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.
Thinking we should test typeof ToolbarChipGroup
before returning categoryName.key
? (It's possible that key
may not be defined for the given object.) That way, the string could be returned as the default.
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 only types valid for categoryName is string or ToolbarChipGroup which requires a key. So it should be safe
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.
Although categoryName
is typed, users can still provide a different object (e.g., categoryName={myObject as any}
). I'm just advocating that it's safer to test for the ToolbarChipGroup
interface first (since you're already using typeof
) and default to string; thus, avoiding a potential undefined exception.
PF4 preview: https://patternfly-react-pr-4553.surge.sh |
a07b801
to
95e61a6
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.
LGTM
Your changes have been released in:
Thanks for your contribution! 🎉 |
Closes #3552