[READY FOR REVIEW] feat(button): added icon support #13

bmdalex · 2018-07-25T15:24:40Z

Button

This PR will introduce possibility to specify the following props for a Button component:

icon=boolean|string: in order to add an icon
iconPosition="before"|"after": in order to specify where the icon is going to be positioned relative to the label

Based on react-old/pull/111 which is now closed

TODO

API Proposal

Icon

A button can be made of only an icon.

<Button type="primary" icon="book" />

generates:

<button>
  <i></i>
</button>

Content and Icon

A button can have an icon in addition to content.

<Button type="primary" icon="book" content="Click me before" iconPosition="before" />
<Button type="secondary" icon="coffee" content="Click me after" iconPosition="after" />

generates:

<button>
  <i></i>
  <span>Click me before</span>
</button>
<button>
  <span>Click me after</span>
  <i></i>
</button>

Circular Emphasis

A button can be circular and formatted to show different levels of emphasis.

<Button type="primary" circular icon="coffee" />
<Button type="secondary" circular icon="book" />

will output

<button>
  <i></i>
</button>
<button>
  <i></i>
</button>

codecov · 2018-07-25T15:24:48Z

Codecov Report

Merging #13 into master will decrease coverage by 0.87%.
The diff coverage is 44.89%.

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   83.64%   82.77%   -0.88%     
==========================================
  Files          59       59              
  Lines         807      830      +23     
  Branches      164      177      +13     
==========================================
+ Hits          675      687      +12     
- Misses        128      139      +11     
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Button/buttonVariables.ts	`100% <ø> (ø)`	⬆️
src/components/Text/textVariables.ts	`0% <0%> (ø)`	⬆️
src/components/Text/textRules.ts	`0% <0%> (ø)`	⬆️
src/components/Text/Text.tsx	`75% <100%> (ø)`	⬆️
src/components/Button/Button.tsx	`82.75% <64.28%> (-17.25%)`	⬇️
src/components/Button/buttonRules.ts	`89.65% <78.57%> (-10.35%)`	⬇️

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 953674c...d3252d4. Read the comment docs.

bmdalex · 2018-07-25T15:42:04Z

@levithomason
these check failure look strange to me:

@codecov codecov/patch — 81.81% of diff hit (target 84.24%) Details
@codecov codecov/project — 83.99% (-0.26%) compared to 2043af5 Details

bmdalex · 2018-07-25T15:38:44Z

docs/src/examples/components/Button/Types/ButtonExampleIcon.tsx

+
+const ButtonExampleIcon = () => (
+  <Button type="primary" style={{ minWidth: '32px', padding: 0 }}>
+    <Icon name="book" color="white" style={{ margin: 'auto' }} />


Adding the comments from previous PR:

@levithomason 6 days ago Member

No inline styles in doc examples please. Let's show exactly what the components do.

@kuzhelov 5 days ago Member

here is a problem with that - this is file that corresponds to Children API case. In other words, it is where Button component is unaware of its children and, thus, in contrast to shorthand case, cannot introduce any appropriate styles to provide a nice look.

At the same time, we would like the presentation of both Shorthand and Children API cases being consistent with each other, so that Button's look won't suddenly change when user is switching between them for the same example (or do we?). In this case it seems that there are no other means to make these looks consistent other than introducing inline styles.

Another possible option for us could be to remove Children API for this example.

@levithomason, please, let us know about your thoughts.

@Bugaa92 a day ago Member

@kuzhelov thanks for detailing the reasons for making the style changes in example;
@levithomason Roman explained very well why inline styles were added, what are your thoughts?

Each component (Button, Icon) should have props for achieving these configurations. Here's an example:

<Button type="primary" icon> <Icon name="book" color="white" fitted /> </Button> <Button type="primary" icon="book" />

Button

The existing icon prop is enough to format the button to house an icon child. It just needs to accept a boolean value in the types.

Icon

Introduce fitted to remove the default margins.

I just want to reiterate, our docs can never have inline styles. If they do, we are telling users that our components cannot achieve certain layouts or features without inline styling. This is, however, the primary objective of Stardust.

bmdalex · 2018-07-26T10:36:26Z

ok, so I'm currently blocked with the following problem

The problem:

For the Children API for a Button with icon and text we cannot achieve correct spacing (as we do for Shorthand API) without adding some padding/margin to the left/right of the icon.

Children API:

generated by:

<Button type="primary" icon>
  <Icon name="book" color="white" fitted />
  <span>Click me left</span>
</Button>
<Button type="secondary" icon>
  <span>Click me right</span>
  <Icon name="coffee" fitted />
</Button>

Shorthand API:

generated by:

<Button type="primary" icon="book" iconPosition="left" content="Click me left" />
<Button type="secondary" icon="coffee" iconPosition="right" content="Click me right" />

Limitation:

For the Children API we don't want to override children elements and when we're using the Icon and span/Text together the spacing (between icon, text and margins) is not correct.

Experiment:

So far we tried adding display: grid / inline-grid on Button but it doesn't work correctly because display: grid is not suitable for buttons according to W3C spec. There's also a bug about it, no idea though what's expected. To make this work, we could use another div (that will act like the grid container) to wrap the children in, as in this Codepen DEMO.

Concerns:

Does this count as manipulating children since the user expects them under the Button component?
Does this still respect accessibility DOM structure?
Isn't it adding too much complexity?

Alternatives:

Seems like we'll have to introduce a special property for the icon; here are some proposals:

margin="left|right": for adding space to the left or right depending on where it's positioned relative to the text; something like:

<Button type="primary" icon>
  <Icon name="book" color="white" margin="right" />
  <span>Click me left</span>
</Button>
<Button type="secondary" icon>
  <span>Click me right</span>
  <Icon name="coffee" margin="left" />
</Button>

spaceAround (boolean|number in px|string): for adding space to left and right and then we'll correct the button padding; something like:

<Button type="primary" icon>
  <Icon name="book" color="white" spaceAround />
  <span>Click me left</span>
</Button>
<Button type="secondary" icon>
  <span>Click me right</span>
  <Icon name="coffee" spaceAround />
</Button>

Thoughts?
@levithomason , @mnajdova , @kuzhelov

brianespinosa · 2018-07-26T15:26:20Z

src/components/Button/Button.tsx

+            name={icon}
+            fitted
+            color={primary ? 'white' : 'black'}
+          />


Where we are adding the <Icon /> component here, I THINK the easiest way to solve your problem is to add the margin prop and have it be the opposite value of the Button's iconPosition prop.

<Icon key="btn-icon" name={icon} fitted color={primary ? 'white' : 'black'} margin={iconPosition === 'left' ? 'right' : 'left'} />```

@Bugaa92 unless I am missing something, I think the above should solve your problem with the shorthand API.

the problem is not with Shorthand API because there I can achieve that by adding space to the span instead by styling it however I want (it's an implementation detail); however, in Children API it's not an implementation detail as it's part of the API the user will have to use to achieve the same styling.

tl;dr
Your comment still holds and it's basically agreeing with my suggestion in the previous comment

margin="left|right": for adding space to the left or right depending on where it's positioned relative to the text; something like:

<Button type="primary" icon> <Icon name="book" color="white" margin="right" /> <span>Click me left</span> </Button> <Button type="secondary" icon> <span>Click me right</span> <Icon name="coffee" margin="left" /> </Button>

I believe it's the best option right now to add that margin to the Icon component;
@levithomason what are your thoughts?

@mnajdova since she hit the same issue

unblocked by adding xSpacing prop to Icon component in #22

bmdalex · 2018-07-30T19:20:04Z

@mnajdova
can you please review, this is ready now

mnajdova · 2018-07-31T09:00:27Z

CHANGELOG.md

+### Features
+- Add Icon `xSpacing` prop @Bugaa92 ([#22](https://github.com/stardust-ui/react/pull/22))
+- Add Button `icon` prop @Bugaa92 ([#13](https://github.com/stardust-ui/react/pull/13))
+


As far as I can see, this PR is also adding the truncated prop for the Text component. Please add that in the CHANGELOG as well.

mnajdova · 2018-07-31T09:01:20Z

docs/src/examples/components/Button/Types/ButtonExample.tsx

@@ -1,6 +1,11 @@
 import React from 'react'
-import { Button } from '@stardust-ui/react'
+import { Button, Icon, Text } from '@stardust-ui/react'


Can you please remove the unused imports.

done, good catch! we need a tslint rule for this

mnajdova · 2018-07-31T09:04:21Z

src/components/Text/Text.tsx

+  success?: boolean
+  timestamp?: boolean
+  truncated?: boolean
+}


Thanks for adding the Typings :)

mnajdova · 2018-07-31T09:45:27Z

src/components/Button/Button.tsx

+      ].filter(Boolean)
+
+      return iconPosition === 'after' ? renderedContent : renderedContent.reverse()
+    }


nit: could replace the condition with just iconIsAfterButton

that was the point, I was sleeping probably; thx :)

bmdalex requested review from dvdzkwsk, kuzhelov and levithomason as code owners July 25, 2018 15:24

bmdalex force-pushed the feat/button-icon branch from 0571fb4 to 3ea8615 Compare July 25, 2018 15:26

bmdalex changed the title ~~feat(button): @Bugaa92 added icon support @kuzhelov addressed review comments~~ [SUGGESTION NEEDED] feat(button): added icon support Jul 25, 2018

This was referenced Jul 25, 2018

[READY FOR REVIEW] feat(Button): adding disabled prop to Button #14

Merged

[SUGGESTION NEEDED] feat(button): added icon support stardust-ui/react-old#111

Closed

bmdalex commented Jul 25, 2018

View reviewed changes

bmdalex force-pushed the feat/button-icon branch from 3ea8615 to 56375c0 Compare July 26, 2018 10:16

bmdalex changed the title ~~[SUGGESTION NEEDED] feat(button): added icon support~~ [WIP][SUGGESTION NEEDED] feat(button): added icon support Jul 26, 2018

brianespinosa reviewed Jul 26, 2018

View reviewed changes

bmdalex force-pushed the feat/button-icon branch from 56375c0 to 7a9e860 Compare July 30, 2018 18:48

bmdalex requested a review from mnajdova as a code owner July 30, 2018 18:48

bmdalex force-pushed the feat/button-icon branch from 7a9e860 to 492b181 Compare July 30, 2018 19:07

bmdalex changed the title ~~[WIP][SUGGESTION NEEDED] feat(button): added icon support~~ [READY FOR REVIEW] feat(button): added icon support Jul 30, 2018

bmdalex force-pushed the feat/button-icon branch 3 times, most recently from b10de4d to be76820 Compare July 30, 2018 20:50

mnajdova reviewed Jul 31, 2018

View reviewed changes

mnajdova approved these changes Jul 31, 2018

View reviewed changes

mnajdova reviewed Jul 31, 2018

View reviewed changes

bmdalex force-pushed the feat/button-icon branch from be76820 to df7320e Compare July 31, 2018 12:05

feat(button): added 'icon' prop for Button component

d3252d4

bmdalex force-pushed the feat/button-icon branch from df7320e to d3252d4 Compare July 31, 2018 12:13

bmdalex merged commit 6795a5d into master Jul 31, 2018

bmdalex deleted the feat/button-icon branch July 31, 2018 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY FOR REVIEW] feat(button): added icon support #13

[READY FOR REVIEW] feat(button): added icon support #13

bmdalex commented Jul 25, 2018 •

edited

codecov bot commented Jul 25, 2018 •

edited

bmdalex commented Jul 25, 2018

bmdalex Jul 25, 2018

levithomason Jul 25, 2018

bmdalex commented Jul 26, 2018 •

edited

brianespinosa Jul 26, 2018

brianespinosa Jul 26, 2018

bmdalex Jul 27, 2018

bmdalex Jul 27, 2018

bmdalex Jul 30, 2018

bmdalex commented Jul 30, 2018

mnajdova Jul 31, 2018

bmdalex Jul 31, 2018

mnajdova Jul 31, 2018

bmdalex Jul 31, 2018

mnajdova Jul 31, 2018

bmdalex Jul 31, 2018

mnajdova Jul 31, 2018

bmdalex Jul 31, 2018

[READY FOR REVIEW] feat(button): added icon support #13

[READY FOR REVIEW] feat(button): added icon support #13

Conversation

bmdalex commented Jul 25, 2018 • edited

Button

TODO

API Proposal

Icon

Content and Icon

Circular Emphasis

codecov bot commented Jul 25, 2018 • edited

Codecov Report

bmdalex commented Jul 25, 2018

Choose a reason for hiding this comment

@levithomason 6 days ago Member

@kuzhelov 5 days ago Member

@Bugaa92 a day ago Member

Choose a reason for hiding this comment

Button

Icon

bmdalex commented Jul 26, 2018 • edited

The problem:

Children API:

Shorthand API:

Limitation:

Experiment:

Concerns:

Alternatives:

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

bmdalex commented Jul 30, 2018

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

bmdalex commented Jul 25, 2018 •

edited

codecov bot commented Jul 25, 2018 •

edited

bmdalex commented Jul 26, 2018 •

edited