[EuiFilterGroup] Add compressed prop#5717
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5717/ |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
Suggestion: How about also decreasing the EuiNotificationBadge size to s when compressed?
| export type EuiFilterButtonProps = Omit< | ||
| EuiButtonEmptyProps, | ||
| 'flush' | 'size' | ||
| > & { |
There was a problem hiding this comment.
On thing that I've noticed with our Props tables in the docs is that it spits out the props in the order that we supply them here. So in this case, the custom filter button props will display after all the EuiButtonEmptyProps. Can I make a suggestion and put this after the filter button props?
There was a problem hiding this comment.
Also removing these props may be considered a breaking change in case users are in fact using this props?
There was a problem hiding this comment.
Yes, this is technically a breaking change
| .euiFilterButton__notification { | ||
| line-height: $euiSize; | ||
| height: $euiSize; | ||
| min-width: $euiSize; | ||
| } |
There was a problem hiding this comment.
Oh right, because you're not applying a size to the filter button itself, there's no way to set size="s" directly on the notification badge.... 🤔
This is kind of a scary override/customization to maintain.
What if it's just always size="s"? And remove any current overrides to the style. It feels a bit large to begin with.
There was a problem hiding this comment.
One thing that I did first was to remove the size="m" and when the .euiFilterGroup:not(.euiFilterGroup--compressed) I was extending the .euiNotificationBadge--medium. But I noticed that we don't use Sass extends regularly so I decided to override. 😬
But just leave the default size="s" looks good to me.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5717/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5717/ |
|
Thanks, @cchaos, and @thompsongl. I went through the review and fixed everything. @cchaos I also updated the Figma component and added a compressed variant. I already published the changes. The notification badges there were already using the size small. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5717/ |
thompsongl
added
the
breaking change
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
Just a couple nits, but otherwise LGTM! Thanks for tackling!
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5717/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5717/ |
4 participants
Summary
This PR adds a
compressedprop to EuiFilterGroup and reduces the size of the EuiFilterButton notification badge. It also closes #5695.As mentioned in #5695, one of the ways to solve this issue was to inherit the EuiButton prop
sizein EuiFilterButton. But IMO supporting different sizes could cause some inconsistencies in usage. Consumers could pass one size to one button and other sizes to others. Also passing a size prop to multiple components seems very a repetitive task when we can achieve the same goal by just passing one prop in the parent component.For this reason, I decided to add a
compressedprop in the EuiFilterButton parent component EuiFilterGroup.Also, the EuiFilterGroup is part of our forms section and I think it makes sense to also support the
compressedprop like other form components.Checklist
[ ] Checked in mobile[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes