-
Notifications
You must be signed in to change notification settings - Fork 433
Fixed a runtime error caused by the Pills Container component trying to access an undefined icon. #3152
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
Conversation
Thanks for opening this pull request! 💯 This is a community-driven project, and we can't do it without your participation. Please check out our contributing guidelines and review the Contributor Checklist if you haven't already, to make sure everything is squared away. CircleCI will take about 10 minutes to run through the same items that are on the Contributor checklist with a pass/fail check below. Please fix any issues that cause CircleCI to fail or ask for clarification--we try, but sometimes the errors can be unclear. |
Thanks for the contribution! Before we can merge this, we need @shadwar123 to sign the Salesforce Inc. Contributor License Agreement. |
Thanks for picking this up! Please remove the package lock file changes unless you are updating dependencies. |
@@ -198,7 +198,7 @@ class Pill extends React.Component { | |||
}; | |||
|
|||
renderIcon = () => { | |||
const icon = this.props.icon || this.props.avatar; | |||
const icon = this.props?.icon || this.props?.avatar; |
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.
What is the situation that this.props
becomes undefined within?
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.
Actually the undefined problem is comming beacuse of selected-listbox.jsx getIcon funtion as In some cases option only have icon or in other case only avatar, there i have applied const iconObject = option?.icon || null; and here const icon = this.props?.icon || this.props?.avatar; for the extra safety.
I see that you downgraded the package lock verison. Please remove your package.lock file. |
Done, |
@shadwar123 You shouldn't delete package-lock files in libraries you are contributing to. Please restore the old version. |
Done. |
Congrats on merging your first pull request to Design System React! 🎉 |
Fixes #3138
Additional description
The Pills Container component was encountering a runtime error because it tried to access an icon that was sometimes undefined. To fix this, I applied optional chaining where the icon is being updated. If the icon is not present, we set its value to null. As a result, the getAvatar() function is called, and the props are passed with icon = null and avatar = value. In the Utilities, there's a Pill component that renders either the icon or the avatar based on their values using render() function.
@interactivellama
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fix
has been run and linting passes.components/component-docs.json
CI tests pass (npm test
).REVIEWER checklist (do not remove)
components/component-docs.json
tests.Required only if there are markup / UX changes
last-slds-markup-review
inpackage.json
and push.last-accessibility-review
, topackage.json
and push.npm run local-update
within locally cloned site repo to confirm the site will function correctly at the next release.