-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(icon-indicator): new component #18191
feat(icon-indicator): new component #18191
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18191 +/- ##
==========================================
+ Coverage 84.15% 84.16% +0.01%
==========================================
Files 406 408 +2
Lines 14407 14428 +21
Branches 4690 4688 -2
==========================================
+ Hits 12124 12144 +20
- Misses 2118 2119 +1
Partials 165 165 ☔ View full report in Codecov by Sentry. |
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 looks awesome! Two main points to call out, plus a few addl things in comments
- The react component needs to be exported from the entrypoint
packages/react/src/index.js
andpackages/react/src/index.ts
- The exports should be marked unstable/experimental for now so we have space to get feedback and alter the API if necessary
export { IconIndicator as unstable__IconIndicator } from './components/IconIndicator';
- Could the story be updated to align the icons into two uniform columns?
packages/react/src/components/IconIndicator/IconIndicator.stories.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/IconIndicator/__tests__/IconIndicator-test.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/IconIndicator/__tests__/IconIndicator-test.js
Show resolved
Hide resolved
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.
Could put an example of these in the custom-theme example if you'd like, alongside the button override.
carbon/examples/custom-theme/src/index.scss
Lines 16 to 19 in acead26
// override a component token | |
@use '@carbon/react/scss/components/button/tokens' as button-tokens with ( | |
$button-primary: #3f51b5 | |
); |
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.
i added it to this PR, but since the styles/component hasn't been released yet it's causing the build to fail. I will remove for now and make a PR once this is in the next release.
packages/web-components/src/components/icon-indicator/icon-indicator.stories.ts
Outdated
Show resolved
Hide resolved
Overall I think this is in good shape for a visual review @thyhmdo 👍 |
@ariellalgilmore This looks great! There's only one thing and this is on my that I didn't check on the name update. But these two status icons' names need tobe updated: Error --> Failed |
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.
Looks great!
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!
03ec6cb
Closes #17539
worked on with @preetibansalui and @2nikhiltom! 🎉
New Icon indicator component
Changelog
New
Changes
Testing / Reviewing
confirm icon indicators look good in react and web component storybooks
confirm tests and coverage are passing