-
Notifications
You must be signed in to change notification settings - Fork 55
[READY FOR REVIEW] feat(button): added icon support #13
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
0571fb4
to
3ea8615
Compare
|
||
const ButtonExampleIcon = () => ( | ||
<Button type="primary" style={{ minWidth: '32px', padding: 0 }}> | ||
<Icon name="book" color="white" style={{ margin: 'auto' }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
3ea8615
to
56375c0
Compare
ok, so I'm currently blocked with the following problem The problem:For the Children API for a 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 Experiment:So far we tried adding Concerns:
Alternatives:Seems like we'll have to introduce a special property for the icon; here are some proposals:
<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>
<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? |
src/components/Button/Button.tsx
Outdated
name={icon} | ||
fitted | ||
color={primary ? 'white' : 'black'} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'}
/>```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bugaa92 unless I am missing something, I think the above should solve your problem with the shorthand API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnajdova since she hit the same issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unblocked by adding xSpacing
prop to Icon component in #22
56375c0
to
7a9e860
Compare
7a9e860
to
492b181
Compare
@mnajdova |
b10de4d
to
be76820
Compare
CHANGELOG.md
Outdated
### 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)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thx
@@ -1,6 +1,11 @@ | |||
import React from 'react' | |||
import { Button } from '@stardust-ui/react' | |||
import { Button, Icon, Text } from '@stardust-ui/react' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please remove the unused imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, good catch! we need a tslint rule for this
success?: boolean | ||
timestamp?: boolean | ||
truncated?: boolean | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the Typings :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure np :)
src/components/Button/Button.tsx
Outdated
].filter(Boolean) | ||
|
||
return iconPosition === 'after' ? renderedContent : renderedContent.reverse() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could replace the condition with just iconIsAfterButton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was the point, I was sleeping probably; thx :)
be76820
to
df7320e
Compare
df7320e
to
d3252d4
Compare
Button
This PR will introduce possibility to specify the following props for a Button component:
icon=boolean|string
: in order to add an iconiconPosition="before"|"after"
: in order to specify where the icon is going to be positioned relative to the labelBased on react-old/pull/111 which is now closed
TODO
API Proposal
Icon
A button can be made of only an icon.
generates:
Content and Icon
A button can have an icon in addition to content.
generates:
Circular Emphasis
A button can be circular and formatted to show different levels of emphasis.
will output