-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
ccfd498
to
b234a65
Compare
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
==========================================
+ Coverage 87.51% 88.13% +0.62%
==========================================
Files 43 43
Lines 729 725 -4
Branches 98 96 -2
==========================================
+ Hits 638 639 +1
+ Misses 88 83 -5
Partials 3 3
Continue to review full report at Codecov.
|
src/components/Button/Button.tsx
Outdated
@@ -105,7 +105,7 @@ class Button extends UIComponent<any, any> { | |||
key="btn-icon" | |||
name={icon} | |||
xSpacing={!content ? 'none' : iconIsAfterButton ? 'before' : 'after'} | |||
color={primary ? 'white' : 'black'} | |||
variables={{ color: primary ? 'white' : 'black' }} |
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, this still will result in Icon having color different from the button's one (obviously, Icon here could be either black or white) - while, as a consumer, I would rather expect that it would take foreground color of Button
b234a65
to
eab2fd5
Compare
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.
as we've agreed upon, will share cases that I asee will be problematic in case
- of using
props
instead of variables, with the limited set of values and much limited set of things available for tweaks. To be clear - limit amount of customization options is absolutely necessary if we would like components to look consistent, but this move to props will eliminate some customization mechanisms that I see being critical - introduce cases where 'inherit' fallback won't be an option - there are quite a few of them
f09feae
to
a2464c1
Compare
a2464c1
to
181cf51
Compare
181cf51
to
628cd19
Compare
This PR touched a few concepts in fixing the color. I have updated it to only fix the issue with passing the correct Icon variable. Broader changes concerning our props vs variables API can be hashed out and handled separately. |
All tests now pass. @kuzhelov, merging since I've reverted the changes you've objected to. |
This PR fixes the icon color inside of Buttons.