fix: Ensure icon color for SystemIconCircle can be overwritten #3322
Conversation
RayRedGoose
requested review from
alanbsmith,
mannycarrera4,
josh-bagwell and
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a color prop to the SystemIconCircle component for manual icon color overrides and exports the systemIconCircleStencil. It also updates the story examples to demonstrate using the new prop.
- Exports
systemIconCircleStenciland addscolorprop with fallback logic toSystemIconCircle - Updates
Icons.stories.tsxto import the stencil and showcolorusage
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| modules/react/icon/stories/Icons.stories.tsx | Imported systemIconCircleStencil, added color usage in story, removed legacy import of base |
| modules/react/icon/lib/SystemIconCircle.tsx | Exported stencil, defined color prop, updated style vars and defaults |
Comments suppressed due to low confidence (4)
modules/react/icon/lib/SystemIconCircle.tsx:31
- New
colorprop behavior isn’t covered by existing unit tests. Consider adding tests to verify the override and fallback logic forcolor.
color?: string;
modules/react/icon/lib/SystemIconCircle.tsx:55
- The code references
base.soap200butbaseis not imported; also, the importedcolorsexport providescolors.soap200. Consider usingcssVar(background, colors.soap200)or explicitly importingbasefrom the correct tokens package.
background: cssVar(background, base.soap200),
modules/react/icon/lib/SystemIconCircle.tsx:55
cssVarandsystemare used in this file but neither is imported. Please addimport {cssVar} from '@workday/canvas-kit-styling'andimport {system} from '@workday/canvas-tokens-web'(or the appropriate modules) so these references resolve.
background: cssVar(background, base.soap200),
modules/react/icon/stories/Icons.stories.tsx:19
- [nitpick] The
createStylesimport is unused in this file. Removing it will clean up unused code.
import {createStyles, cssVar} from '@workday/canvas-kit-styling';
Workday/canvas-kit
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
fix-system-icon-circle
|
| Run status |
|
| Run duration | 02m 49s |
| Commit |
|
| Committer | Raisa Primerova |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
21
|
Skipped
|
0
|
Passing
|
936
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
21.27%
|
|
|---|---|
|
Untested elements
|
1530
|
|
Tested elements
|
411
|
Accessibility
99.28%
|
|
|---|---|
|
Failed rules
|
6 critical
5 serious
0 moderate
2 minor
|
|
Failed elements
|
98
|
|
This feels like a feature cause you're adding stuff, but it's a fix into support yeah? |
| * If not specified, it will be calculated based on the background color. | ||
| * @default rgba(0,0,0,0.65) | ||
| */ | ||
| color?: string; |
There was a problem hiding this comment.
ah maybe you're just matching with SystemIcon does.
| vars: { | ||
| containerSize: '', | ||
| backgroundColor: '', | ||
| iconColor: '', | ||
| background: '', |
There was a problem hiding this comment.
i suppose not a breaking change because it wasn't previously exported?
There was a problem hiding this comment.
yes, there is nothing used externally before 👌
|
@mannycarrera4 I feel like it's tricky :D i'm adding some stuff, but the purpose is to fix issue with icon colors, that is not work anymore with vars 🤔 |
|
Maybe we make this off of master instead? |
|
Yeah let's do |
mannycarrera4
changed the title
mannycarrera4
added
automerge
and removed
ready for review
Summary
Fixes: #3321
Ensure Icon color is the same for all background colors.
Release Category
Components
Release Note
Property
colorhas been added toSystemIconCircleto fix and allow the overwriting of icon color if it's not handle automatically.systemIconCircleStencilis now exported.Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)