[EuiButtonEmpty] Reduce icon in xs size #4759
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4759/ |
There was a problem hiding this comment.
Thanks @andreadelrio! 🎉
It makes sense to have a smaller icon for the EuiButtonEmpty when the size is xs.
I just have a few code suggestions.
| @@ -67,6 +68,7 @@ export const EuiButtonContent: FunctionComponent< | |||
| textProps, | |||
| isLoading = false, | |||
| iconType, | |||
| size, | |||
There was a problem hiding this comment.
The goal of this size prop is to change the EuiIcon size so it should be called iconSize. This way on the parent component we know it will only control the icon size. Also, we can tell what is the default size.
| size, | |
| iconSize = 'm', |
There was a problem hiding this comment.
Instead of passing in iconSize should we allow all of iconProps? (We could still restrict the size prop within it too.)
Just wanted to bring it up because in other places where expose some but not all of an underlying component's props, we sometimes end up getting more requests to expose more and more of the API and then we end up with a messy hybrid system.
There was a problem hiding this comment.
Instead of passing in iconSize should we allow all of iconProps
It's a good question. This component is internal (not exported as part of EUI's top-level API), so any decisions here won't impact consumers or result in requests.
If we wanted to merge the icon props into iconProps, it'd be EuiIconProps + iconSide? Or iconSide would remain by itself?
Also, we could merge this and look at the API some more; we can change it as many times as we want without external impact.
There was a problem hiding this comment.
The <EuiButtonContent /> is an inner component used in EuiButton and EuiButtonEmpty. It's not meant to be used by consumers, but I agree that we should be more consistent to not end up with a messy hybrid system. 🙂
With that said, if @andreadelrio is ok to refactor the code, I'm up for it.
There was a problem hiding this comment.
I added this comment at the same time that @thompsongl added his comment. After reading his opinion I think is better to merge the PR as it is.
There was a problem hiding this comment.
I didn't realize these are internal components in which case it's a lot easier to iterate on the API so merge away!
|
Good ideas all around, @miukimiu. That's a better way to handle it. |
|
@miukimiu thanks for the suggestions! much cleaner. I made the changes. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4759/ |
Summary
Reduce size of the icon in
EuiButtonEmptyso that when the size isxsthe icon's size iss.This is one of the issues we're tracking on the Amsterdam Airtable.
Checklist
- [ ] Props have proper autodocs and **[playground toggles](https://github.com/elastic/eui/blob/master/wiki/documentation-guidelines.md#adding-playground-toggles)**- [ ] Added documentation- [ ] Checked Code Sandbox works for the any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes