-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Icon): revert changes with different roots #1435
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1435 +/- ##
==========================================
- Coverage 73.59% 73.53% -0.07%
==========================================
Files 787 787
Lines 5890 5894 +4
Branches 1738 1722 -16
==========================================
- Hits 4335 4334 -1
- Misses 1549 1554 +5
Partials 6 6
Continue to review full report at Codecov.
|
….com/stardust-ui/react into fix/revert-icon-roots
@@ -3,7 +3,7 @@ import { IconProps } from '../../../../components/Icon/Icon' | |||
import { IconVariables } from '../../../teams/components/Icon/iconVariables' | |||
|
|||
const iconStyles: ComponentSlotStylesInput<IconProps, IconVariables> = { | |||
fontRoot: (): ICSSInJSStyle => ({ | |||
root: (): ICSSInJSStyle => ({ |
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.
Shuold we add this conditionally if the icon is font icon? Won't this break icons on docs?
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 check that it doesn't break anything, so not sure if you should implement the comment...
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.
agree that it is better to introduce explicit condition check here, as we are expecting this rule to be applied exclusively to font-based icons
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.
👍
….com/stardust-ui/react into fix/revert-icon-roots # Conflicts: # CHANGELOG.md
Fixes #1407.
Reverts changes with multiple roots from #1337 (
svgRoot
&fontRoot
). This idea breaks ability to override styles viastyles
prop onIcon
component:svgRoot
&fontRoot
had more priority on merge. Also we usedBox
component there, it's redundant.We definitely will want to separate these styles somehow to avoid conditions, but
svgRoot
/fontRoot
is definitely bad stuff.