fix(Button): icon colors and layout #135
Conversation
kuzhelov
requested review from
dvdzkwsk,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #135 +/- ##
==========================================
+ Coverage 89.04% 89.05% +0.01%
==========================================
Files 47 47
Lines 776 786 +10
Branches 101 113 +12
==========================================
+ Hits 691 700 +9
- Misses 83 84 +1
Partials 2 2
Continue to review full report at Codecov.
|
kuzhelov
added
🚀 ready for review
🧰 fix
kuzhelov
changed the title
| @@ -25,6 +26,7 @@ export default (siteVars: any): IButtonVariables => { | |||
| height: pxToRem(32), | |||
| minWidth: pxToRem(96), | |||
| maxWidth: pxToRem(280), | |||
| color: undefined, | |||
There was a problem hiding this comment.
Seems strange to have an undefined variable that is also never set to a value. Is there not a siteVariable available for this value?
In either case, what is the motivation for this variable?
There was a problem hiding this comment.
the motivation for this variable is to allow to customize Button's color (where 'color' comes in general sense, not something that is tied to corresponding CSS property only). This will open way for the following customization scenarios, as well as allow to customize this aspect of Button component in docs
<Button variables={{ color: 'red' }} ... />Seems strange to have an undefined variable that is also never set to a value. Is there not a siteVariable available for this value?
Yes, let me initialise it with an appropriate site variable - the reason haven't done it before is because it was unclear to me at this moment which ones will be provided by teams theme
| : type === 'secondary' | ||
| ? variables.typeSecondaryColor | ||
| : variables.color, | ||
| }, |
There was a problem hiding this comment.
Any variables or styles used directly in a component's logic implicitly become part of a global theming interface.
All themes will have to implement variables.typePrimaryColor, for instance, in order for this to work. Additionally, we are removing the styling from the theme and baking it into the component instead.
I think all theming values and theming logic should live in the theme's files. This way, each theme is free to choose the values and logic behind their styles. The question is how?
Proposal 1 - Component part variables
Component variables files could define variables for each component parts as well. Then, variable values and logic for component parts can be left to the theme and not dictated to all themes by the component. Here's an example of the Button theme defining an icon part in its variables.
// themes/teams/components/Button/buttonVariables.tsx
export default (siteVariables) => {
const variables = {
color: undefined,
typePrimaryColor: siteVariables.white,
typeSecondaryColor: siteVariables.black,
}
variables.icon = {
color:
type === "primary"
? variables.typePrimaryColor
: type === "secondary"
? variables.typeSecondaryColor
: variables.color
}
return variables
}Now, the component simply implements a contract that says "we pass variables[part] to each component part":
// Button.tsx
Icon.create(icon, {
defaultProps: {
variables: variables.icon
}
})This would also allow us to do this consistently for all components, including the styles for each component part. That then would allow us to write a conformance test for it. This would make the API easier to understand and work with as well since there is only one concept, "components pass variables/styles to each component part".
Proposal 2 - A high-level theme interface
This proposal would introduce a new concept. Something like a lightweight theme interface that is allowed to pass between components. It would consist of backgroundColor, foregroundColor, and perhaps an accentColor initially (names and keys are inspired by present convergence talks with several teams at Microsoft).
The Button in this case would define these three values for its bounds. Then, they would passed to the Icon.
There are a lot more considerations here, but I'm less in favor of this pattern for the problem at hand so I'll leave it here for now.
Thoughts?
There was a problem hiding this comment.
yes, agree with the reasons provided above, thanks - this goes in line with the exactly the same discussion we've had with Miro. For the sake of avoiding to introduce additional complexity at that moment we've decided to move forward with the simplest approach that would indicate our intent to pass variables to child component, and after that introduce changes in a way that theming aspects will be completely separated from logic.
Speaking of the options suggested - I would definitely support the first one, as it suggest clear path that not changes but extends approach we are using now - and, thus, it is not associated with significant complexity increase. Also, with this one we will be able to address quite broad set of scenarios we are currently struggling with (or using the approach where variables are coupled with implementation)
Not saying 'no' to the second option, though - but would rather wait once this concept will solidify in our minds, for the sake of us being absolutely sure about what we are buying for additional efforts laid in this direction, as well as whether this concept is able to cover the cases we are interested in (and how flexible it would be to cover potentially emerged edge-cases)
There was a problem hiding this comment.
for the sake of progressing with that, let me suggest the following steps:
- leave the logic as it is for this PR - as from the consumer perspective provided changes introduce several important fixes
- introduce RFC for those concerns that were raised about how variables should be passed to child components ([RFC] Decouple variables passed to child components from parent component's logic #162 )
- implement related changes by means of dedicated PR (and, as part of this work, also ensure that improved approach is used at all places where variables were passed in the component's logic)
This will allow us to progress with fixes, as well as split these subsequent changes so that it would be easier to review them.
Please, let me know what do you think about it. Thanks!
src/components/Button/Button.tsx
Outdated
| const { children, content, disabled, iconPosition } = this.props | ||
| const hasChildren = childrenExist(children) | ||
|
|
||
| if (hasChildren) { | ||
| return children | ||
| } |
There was a problem hiding this comment.
If I now do:
<Button>Click me</Button>Won't this result in rendering the text "Click me" only? We'd still want to wrap the text in an ElementType and apply our classes/accessibility/rest props.
Codecov Report
@@ Coverage Diff @@
## master #135 +/- ##
==========================================
+ Coverage 67.71% 68.02% +0.31%
==========================================
Files 101 101
Lines 1363 1370 +7
Branches 261 269 +8
==========================================
+ Hits 923 932 +9
+ Misses 438 436 -2
Partials 2 2
Continue to review full report at Codecov.
|
TODO
Introduces changes that are aimed to solve the following problems with
Button.Colors
No color for default button
This customization parameter was absent for default button - although there was
backgroundColorvariable.was (irregardless of value provided)
now
Icon's color of primary/secondary Button
Was not inherited - was hardcoded to be either black or white.
was
now
Styles vanished in disabled state
All button in disabled state were displayed as grey - styling system hasn't honored any colors of enabled look of the button.
was
now
Layout
Layout styles were generally simplified, problems for the following cases were addressed:
iconPosition='before'
was
now
iconPosition='after'
was
now