Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

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

Merged
merged 9 commits into from
Jul 18, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4b7fe95
fix rules logic of icon component
Jul 13, 2018
e15f049
introduce bordered and circular variants
Jul 13, 2018
f339689
remove 'important' from css property values
Jul 13, 2018
6363807
inline Icon examples
Jul 18, 2018
215672f
introduce customization variables
Jul 18, 2018
30df07c
Merge branch 'master' into feat/icon-circular-and-bordered-variants
Jul 18, 2018
4f8b1bd
advertise changes in CHANGELOG
Jul 18, 2018
dc2fa25
make Icon examples being shorthand ones
Jul 18, 2018
b9b291b
Merge branch 'master' into feat/icon-circular-and-bordered-variants
kuzhelov Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions docs/src/examples/components/Icon/Variations/IconExampleBordered.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React from 'react'
import { Icon } from 'stardust'

const examples = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'chess rock',
'book',
'expand',
'play',
'stop',
'calendar alternate outline',
'coffee',
'compass outline',
'area chart',
]

const IconExampleBordered = () => (
<div>{examples.map(iconName => <Icon key={iconName} name={iconName} bordered />)}</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

export default IconExampleBordered
20 changes: 20 additions & 0 deletions docs/src/examples/components/Icon/Variations/IconExampleCircular.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React from 'react'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { Icon } from 'stardust'

const examples = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'chess rock',
'book',
'expand',
'play',
'stop',
'calendar alternate outline',
'coffee',
'compass outline',
'area chart',
]

const IconExampleCircular = () => (
<div>{examples.map(iconName => <Icon key={iconName} name={iconName} circular />)}</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

export default IconExampleCircular
10 changes: 10 additions & 0 deletions docs/src/examples/components/Icon/Variations/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ const Variations = () => (
description="An icon can be formatted with different colors."
examplePath="components/Icon/Variations/IconExampleColored"
/>
<ComponentExample
title="Bordered"
description="An icon can be formatted to appear with rectangular border."
examplePath="components/Icon/Variations/IconExampleBordered"
/>
<ComponentExample
title="Circular"
description="An icon can be formatted to appear circular."
examplePath="components/Icon/Variations/IconExampleCircular"
/>
</ExampleSection>
)

Expand Down
8 changes: 7 additions & 1 deletion src/components/Icon/Icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ class Icon extends UIComponent<any, any> {
/** An element type to render as (string or function). */
as: customPropTypes.as,

/** Icon can appear with rectangular border. */
bordered: PropTypes.bool,

/** Icon can appear as circular. */
circular: PropTypes.bool,

/** Additional classes. */
className: PropTypes.string,

Expand Down Expand Up @@ -43,7 +49,7 @@ class Icon extends UIComponent<any, any> {
size: PropTypes.oneOf(['mini', 'tiny', 'small', 'large', 'big', 'huge', 'massive']),
}

static handledProps = ['as', 'className', 'color', 'kind', 'name', 'size']
static handledProps = ['as', 'bordered', 'circular', 'className', 'color', 'kind', 'name', 'size']

static defaultProps = {
as: 'i',
Expand Down
13 changes: 12 additions & 1 deletion src/components/Icon/iconRules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,17 @@ const getIcon = (kind, name) => {

const getSize = size => `${sizes.get(size)}em` || '1em'

const getBorderedStyles = circular => ({
lineHeight: '1',
padding: '0.5em 0',
boxShadow: '0 0 0 0.1em rgba(0,0,0,.1) inset',
width: '2em',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense. Thanks!

height: '2em',
...(circular ? { borderRadius: '500em' } : { verticalAlign: 'baseline' }),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator Author

@kuzhelov kuzhelov Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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.

})

const iconRules = {
root: ({ color, kind, name, size }) => {
root: ({ props: { color, kind, name, size, bordered, circular } }) => {
const { fontFamily, content } = getIcon(kind, name)
return {
fontFamily,
Expand All @@ -56,6 +65,8 @@ const iconRules = {
boxSizing: 'inherit',
background: '0 0!important',
},

...((bordered || circular) && getBorderedStyles(circular)),
}
},
}
Expand Down