-
Notifications
You must be signed in to change notification settings - Fork 434
Patch request for Badge
component
#1805
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
…ing up the codebase locally
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.
Took a quick look and pointed out some things I've learnt while implementing components as well :)
Also, don't forget to add your component's meta description in package.json
, under the "components" key. You can refer to here, point 3 last step.
@tanhengyeow, I have resolved whatever changes suggested by you. Can you check it out again? |
Hi, @interactivellama I have made the code changes and now is passing all of the 4 checks. Can you review it? |
/** | ||
* Path to the icon. This will override any global icon settings. | ||
*/ | ||
iconPath: PropTypes.string, |
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.
This is usually handled by an higher order component called IconSettings
/** | ||
* Name of the icon category. Visit <a href="http://www.lightningdesignsystem.com/resources/icons">Lightning Design System Icons</a> to reference icon categories. | ||
*/ | ||
iconCategory: requiredIf( |
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.
Many of the older components do use iconCategory
and iconName
but the best pattern is to pass in a whole <Icon>
so that the Icon
props do not have to be duplicated within badge also https://github.com/salesforce/design-system-react/blob/master/docs/codebase-overview.md#reuse-existing-component-props-by-using-component-no-button-iconclassname-
It looks like there are special classnames for badge that are on placed on the icon, so you may want to duplicate the icon passed into a prop and the classNames that way before render.
Please update.
Thanks for the review. I will push the required changes in 3-4 hours. |
This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you. |
This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you. |
I would like to work on this as it could be useful for me in adding Summary Details component. |
Added support for Badge component. @interactivellama @futuremint, can you please review it and suggest some changes. I am up for any changes suggested.
Additional description
I was going through the code base and for better understanding, I add the
Badge
component from https://www.lightningdesignsystem.com/components/badges/ toSLDS
Props taken
color
- Color of the badgeclassName
- CSS classes that are applied to the spaniconCategory
- Name of the icon categoryiconClassName
- CSS classes to be added to iconiconName
- Name of the iconiconPath
- Path to the iconiconPosition
- Position of the iconid
- HTML id applied on the badge spaniconTitle
- Description of iconPreview
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.