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

bmdalex · 2018-07-18T15:31:44Z

Button

A standard button.

TODO

API Proposal

Icon

A button can be made of only an icon.

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

will output

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

Labeled Icon

A button can use an icon as a label.

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

will output

<button>
  <i></i>
  <span>Click me left</span>
</button>
<button>
  <span>Click me right</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-18T15:32:00Z

Codecov Report

Merging #111 into master will increase coverage by 0.29%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
+ Coverage    69.8%   70.09%   +0.29%     
==========================================
  Files          75       76       +1     
  Lines        1202     1234      +32     
  Branches      228      240      +12     
==========================================
+ Hits          839      865      +26     
- Misses        358      364       +6     
  Partials        5        5

Impacted Files	Coverage Δ
src/components/Button/buttonVariables.ts	`100% <ø> (ø)`	⬆️
src/components/Icon/iconVariables.ts	`100% <ø> (ø)`	⬆️
src/components/Icon/iconRules.ts	`90.47% <ø> (ø)`	⬆️
src/styles/customCSS.ts	`100% <100%> (ø)`
src/components/Button/Button.tsx	`85.18% <84%> (-14.82%)`	⬇️
src/components/Button/buttonRules.ts	`93.93% <90.47%> (-6.07%)`	⬇️

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 9b7f612...ab457b8. Read the comment docs.

bmdalex · 2018-07-18T16:26:35Z

ready for review
@levithomason
@mnajdova
@kuzhelov

levithomason

I didn't quite finish getting through the components. I will do so tomorrow. Here are some comments for now.

levithomason · 2018-07-19T05:34:27Z

 - Add Avatar component @mnajdova ([#75](https://github.com/stardust-ui/react/pull/75))
 - Add Menu `shape` property for describing the shape of the component, instead using the type property @mnajdova ([#68](https://github.com/stardust-ui/react/pull/68))
 - Add Input component @alinais ([#64](https://github.com/stardust-ui/react/pull/64))
+- Add Icon Button component @Bugaa92 ([#111](https://github.com/stardust-ui/react/pull/111))


Suggest "Add icon prop to the Button component". Some libraries have actual IconButton components.

levithomason · 2018-07-19T05:36:48Z

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


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

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.

@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 · 2018-07-19T05:45:09Z

+import React from 'react'
+import { Button } from 'stardust'
+
+const ButtonExampleLabeledIcon = () => (


Labeled icon means something different in Semantic, suggest calling this "Icon and Content". Buttons can have labels and they can style the icon as a label resulting in a "labeled icon button". We can save that for another discussion and PR.

levithomason · 2018-07-19T05:47:49Z

+    </Button>
+    <Button type="secondary">
+      <span style={{ paddingRight: '10px' }}>Click me right</span>
+      <Icon name="coffee" style={{ margin: 'auto' }} />


Reminder, no inline styles.

levithomason · 2018-07-19T05:52:22Z

    <ComponentExample
      title="Default"
-      description="A default Button."
+      description="A standard button."


We should standardize these. I'd propose "default", but would like to hear what the rest of the team thinks.

my vote is for default - this is consistent with the title

levithomason · 2018-07-19T05:54:39Z


+    /** Add an Icon by name, props object, or pass an <Icon />. */
+    icon: customPropTypes.some([
+      PropTypes.bool,


Boolean might be cruft here.

kuzhelov · 2018-07-20T15:33:16Z

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

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.

bmdalex · 2018-07-25T16:01:07Z

Moved to new repo: stardust-ui/react/pull/13

bmdalex requested review from kuzhelov, levithomason and mnajdova as code owners July 18, 2018 15:31

bmdalex force-pushed the feat/button-icon branch 2 times, most recently from 4b0f985 to 008d1ce Compare July 18, 2018 19:28

bmdalex mentioned this pull request Jul 18, 2018

feat(Button): adding disabled prop to Button #114

Closed

11 tasks

levithomason suggested changes Jul 19, 2018

View reviewed changes

bmdalex requested a review from dvdzkwsk as a code owner July 20, 2018 13:26

kuzhelov changed the title ~~feat(button): added icon support~~ [SUGGESTION NEEDED] feat(button): added icon support Jul 20, 2018

bmdalex and others added 3 commits July 24, 2018 15:09

feat(button): added icon support

7decff6

address review comments

e6bbf47

introduce comsistent name for stardust import in docs

ab457b8

bmdalex force-pushed the feat/button-icon branch from dd8dc7d to ab457b8 Compare July 24, 2018 13:11

bmdalex mentioned this pull request Jul 25, 2018

[READY FOR REVIEW] feat(button): added icon support microsoft/fluent-ui-react#13

Merged

11 tasks

bmdalex closed this Jul 25, 2018

bmdalex deleted the feat/button-icon branch July 25, 2018 16:03

Conversation

bmdalex commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Button

TODO

API Proposal

Icon

Labeled Icon

Circular Emphasis

Uh oh!

codecov Bot commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bmdalex commented Jul 18, 2018

Uh oh!

levithomason left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov commented Jul 20, 2018

Uh oh!

bmdalex commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bmdalex commented Jul 18, 2018 •

edited

Loading

codecov Bot commented Jul 18, 2018 •

edited

Loading

kuzhelov Jul 20, 2018 •

edited

Loading