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

[bmdalex]

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

• Conformance test
• Minimal doc site example
• Stardust base theme
• Teams Light theme
• Teams Dark theme
• Teams Contrast theme
• Confirm RTL usage
• W3 accessibility check
• Stardust accessibility check
• Update glossary props table
• Update the CHANGELOG.md

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]

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]

@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]

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?

[levithomason]

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]

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]

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'}
/>```

[brianespinosa]

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

[bmdalex]

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?

[bmdalex]

@mnajdova since she hit the same issue

[bmdalex]

unblocked by adding xSpacing prop to Icon component in #22

[bmdalex]

@mnajdova
can you please review, this is ready now

[mnajdova]

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.

[bmdalex]

done, thx

[mnajdova]

Can you please remove the unused imports.

[bmdalex]

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

[mnajdova]

Thanks for adding the Typings :)

[bmdalex]

sure np :)

[mnajdova]

nit: could replace the condition with just iconIsAfterButton

[bmdalex]

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