fix(Icon): color and borderColor should inherit the color, or be changed via the variables #569
Conversation
-added more examples in the color examples of the icons
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
miroslavstastny
as code owners
mnajdova
changed the title
| <Icon name="home" variables={{ color: 'red' }} /> | ||
| <Icon name="home" variables={{ color: 'orange' }} /> | ||
| {/* The colors for the icon and the border are inherited. */} | ||
| <div style={{ color: 'violet', padding: '1rem', display: 'inline-block' }}> |
There was a problem hiding this comment.
May be we can use our Grid component to avoid styling (display, padding)?
<Grid columns='repeat(4, auto)'>
<div />
<div />
// others
</Grid>There was a problem hiding this comment.
We can also split this to 4 four examples and may be this will better option. For example, https://react.semantic-ui.com/collections/menu/#types-pointing
This section has two examples and we can do the same. This will allow to have description in the title instead of code.
There was a problem hiding this comment.
Yep, it makes sense to have four different examples. Will refactor this. Thanks!
There was a problem hiding this comment.
We decided offline, that we will use the Grid with description for each of the examples, as introducing another sections will introduce unnecessary additional scrolling on the page.
| @@ -11,7 +11,7 @@ const Variations = () => ( | |||
| /> | |||
| <ComponentExample | |||
| title="Color" | |||
| description="An icon can have a different color" | |||
| description="An icon is inheriting the color, but can have a different color if provided by the user." | |||
There was a problem hiding this comment.
nit:
... is inheriting color by default, ...
| @@ -17,6 +17,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Fixes | |||
| - The border color of the Icon is inherited if no value is provided for the `color` and `borderColor` variables @mnajdova ([#569](https://github.com/stardust-ui/react/pull/569)) | |||
There was a problem hiding this comment.
please, ensure that this entry will be in a proper section of CHANGELOG.md after merging the recent master
There was a problem hiding this comment.
👍 please, just address the layout issue @layershifter has mentioned before merging
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
| </span> | ||
| } | ||
| weight="bold" | ||
| /> |
There was a problem hiding this comment.
In the future, try to minimize the number of lines of code in doc site examples:
<Text
content={
<span>
USING THE <code>color</code> VARIABLE:
</span>
}
weight="bold"
/><Text weight="bold">USING THE <code>color</code> VARIABLE:</Text>
This PR fixes #506 following the instructions in the #506 (comment)
Here are the rules that are applied:
colorandborderColor, both are inheritedcolorvariable is provided, then both the color of the icon as well as the border are respecting that valueborderColorvariable is provided then the color of the border is respecting this valueIn addition to this, the example for the color of the icon is updated, so that we can catch any regression for the behaviors of the colors. It looks like this now:
