feat(Icon): introduce 'bordered' and 'circular' variants #85

kuzhelov · 2018-07-13T12:07:40Z

Icon, 'bordered' and 'circular' variants

This variants are necessary to introduce border to icons - with 'circular' being an option to introduce rounded border.

TODO

API Proposal

bordered: boolean

<Icon bordered name="..." />

circular: boolean

<Icon circular name="..." />

codecov · 2018-07-13T12:08:01Z

Codecov Report

Merging #85 into master will increase coverage by <.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   69.94%   69.94%   +<.01%     
==========================================
  Files          73       74       +1     
  Lines        1191     1198       +7     
  Branches      223      205      -18     
==========================================
+ Hits          833      838       +5     
- Misses        353      355       +2     
  Partials        5        5

Impacted Files	Coverage Δ
src/components/Icon/iconVariables.ts	`100% <100%> (ø)`
src/components/Icon/Icon.tsx	`100% <100%> (ø)`	⬆️
src/components/Icon/iconRules.ts	`90.47% <66.66%> (-9.53%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e89994...b9b291b. Read the comment docs.

mnajdova · 2018-07-16T14:14:42Z

src/components/Icon/iconRules.ts

+  boxShadow: '0 0 0 0.1em rgba(0,0,0,.1) inset',
+  width: '2em',
+  height: '2em',
+  ...(circular ? { borderRadius: '500em' } : { verticalAlign: 'baseline' }),


It seems to me that with this implementation, we cannot have circular icon without border. I think the current circular example, should be result of using the circular as well as the bordered props. What do you think @kuzhelov ?

actually, this question is a bit tricky. First of all, lets consider the fact that currently we have just one color prop that defines icon's color. Icon's background is transparent. With that being said, it turns out that there is no much value for us to introduce circular button without border - it won't have any visual difference for the client in comparison to the rectangular one.

So, while I totally see your point, it seems that with current set of props we should mix bordered and circular aspects in the circular prop. At the same time, to obtain more flexibility around styling, as well as for the sake of decoupling these aspects, I would suggest the following:

introduce backgroundColor and borderColor variables to further customize Icon's appearance, split circular from bordered. This is a step that could be considered in future, as long as we'll finish with the first round of necessary components.

@mnajdova, @levithomason, what are your thoughts about it?

...there is no much value for us to introduce circular button without border - it won't have any visual difference for the client...

Agreed. Let's not do that. The circular prop should be enough to cause the icon to appear circular.

...This is a step that could be considered in future, as long as we'll finish with the first round of necessary components.

Also agree. Let's get this in and then iterate.

levithomason

This is a public facing change. Please update the CHANGELOG.md.

There should be an entry for this PR added under Features in UNRELEASED. See the instructions there.

levithomason · 2018-07-17T18:38:29Z

docs/src/examples/components/Icon/Variations/IconExampleBordered.tsx

+import React from 'react'
+import { Icon } from 'stardust'
+
+const examples = [


nit: Since these are icon names, let's name this variable iconNames for more clarity.

levithomason · 2018-07-17T18:39:04Z

docs/src/examples/components/Icon/Variations/IconExampleCircular.tsx

+import React from 'react'
+import { Icon } from 'stardust'
+
+const examples = [


As above, let's use iconNames here as well.

levithomason · 2018-07-17T18:44:43Z

docs/src/examples/components/Icon/Variations/IconExampleBordered.tsx

+]
+
+const IconExampleBordered = () => (
+  <div>{examples.map(iconName => <Icon key={iconName} name={iconName} bordered />)}</div>


For simplicity and readability in the example, let's not map over these. Instead, just use multiple Icon components. This makes a shorter example file and requires less thinking to understand what is happening. It also removes the need to use a key.

levithomason · 2018-07-17T18:45:10Z

docs/src/examples/components/Icon/Variations/IconExampleCircular.tsx

+]
+
+const IconExampleCircular = () => (
+  <div>{examples.map(iconName => <Icon key={iconName} name={iconName} circular />)}</div>


As above, let's inline the Icons instead of mapping over a list.

levithomason · 2018-07-17T18:51:57Z

src/components/Icon/iconRules.ts

+  boxShadow: '0 0 0 0.1em rgba(0,0,0,.1) inset',
+  width: '2em',
+  height: '2em',
+  ...(circular ? { borderRadius: '500em' } : { verticalAlign: 'baseline' }),


...there is no much value for us to introduce circular button without border - it won't have any visual difference for the client...

Agreed. Let's not do that. The circular prop should be enough to cause the icon to appear circular.

...This is a step that could be considered in future, as long as we'll finish with the first round of necessary components.

Also agree. Let's get this in and then iterate.

mnajdova · 2018-07-18T10:31:43Z

docs/src/examples/components/Icon/Variations/IconExampleCircular.tsx

@@ -0,0 +1,18 @@
+import React from 'react'


I think this should be a shorthand example (meaning to have this renamed to IconExampleCircular.shorthand.tsx). We don't really have other way for defining this using children, so I think this example should have just shorthand API. What do you think? Maybe @Bugaa92 will have the correct answer. If this is the case, that we should rename all examples that don't have a children, as a shorthand example. Correct me if I am wrong, because that is the approach I have taken on the examples regarding the Menu component.

yes, it seems to be consistent with its semantics - let me do that

mnajdova · 2018-07-18T10:38:11Z

src/components/Icon/iconRules.ts

+  lineHeight: '1',
+  padding: '0.5em 0',
+  boxShadow: `0 0 0 0.1em ${borderColor || color || 'black'} inset`,
+  width: '2em',


Are this going to be dependent on the size prop in the future?

not absolutely sure, frankly - should made additional experiments with it. In either case, even if there would be any dependency between these two, it should be driven by visual looks provided by experiments. So far for the range of regular sizes that are usually used it seems to work fine. Lets address this point in a dedicated PR - what do you think?

Sure, makes sense. Thanks!

requested changes are provided

Roman Kuzhelev added 2 commits July 13, 2018 13:07

fix rules logic of icon component

4b7fe95

introduce bordered and circular variants

e15f049

kuzhelov requested review from levithomason and mnajdova as code owners July 13, 2018 12:07

remove 'important' from css property values

f339689

kuzhelov changed the title ~~feat(Icon): introduce bordered and circular variants~~ feat(Icon): introduce 'bordered' and 'circular' variants Jul 13, 2018

mnajdova reviewed Jul 16, 2018

View reviewed changes

kuzhelov mentioned this pull request Jul 17, 2018

[SUGGESTION NEEDED] feat(Icon): introduce 'background' property #86

Closed

11 tasks

levithomason previously requested changes Jul 17, 2018

View reviewed changes

Roman Kuzhelev added 4 commits July 18, 2018 12:09

inline Icon examples

6363807

introduce customization variables

215672f

Merge branch 'master' into feat/icon-circular-and-bordered-variants

30df07c

advertise changes in CHANGELOG

4f8b1bd

mnajdova reviewed Jul 18, 2018

View reviewed changes

make Icon examples being shorthand ones

dc2fa25

mnajdova approved these changes Jul 18, 2018

View reviewed changes

Merge branch 'master' into feat/icon-circular-and-bordered-variants

b9b291b

kuzhelov merged commit 5ac3ded into master Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Icon): introduce 'bordered' and 'circular' variants #85

feat(Icon): introduce 'bordered' and 'circular' variants #85

kuzhelov commented Jul 13, 2018 •

edited

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading

mnajdova Jul 16, 2018

kuzhelov Jul 16, 2018 •

edited

Loading

levithomason Jul 17, 2018

levithomason left a comment

levithomason Jul 17, 2018

levithomason Jul 17, 2018

levithomason Jul 17, 2018

levithomason Jul 17, 2018

levithomason Jul 17, 2018

mnajdova Jul 18, 2018

kuzhelov Jul 18, 2018

mnajdova Jul 18, 2018

kuzhelov Jul 18, 2018

mnajdova Jul 18, 2018

feat(Icon): introduce 'bordered' and 'circular' variants #85

feat(Icon): introduce 'bordered' and 'circular' variants #85

Conversation

kuzhelov commented Jul 13, 2018 • edited Loading

Icon, 'bordered' and 'circular' variants

TODO

API Proposal

bordered: boolean

circular: boolean

codecov bot commented Jul 13, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

kuzhelov Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov commented Jul 13, 2018 •

edited

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading

kuzhelov Jul 16, 2018 •

edited

Loading