-
Notifications
You must be signed in to change notification settings - Fork 843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix responsiveness of EuiFilterGroup #1849
Conversation
@snide Do you playing with this a bit to see if it suits? Don't look at the code just yet, I have to do cleanup. |
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.
Didn't test IE, but this wrap works much better and seems to scale. Small comments below.
@@ -91,21 +91,20 @@ export default class extends Component { | |||
numFilters={items.length} | |||
hasActiveFilters={true} | |||
numActiveFilters={2} | |||
grow={true} | |||
> | |||
Composers |
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.
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.
I don't think adding title attributes to every filter button would be annoying because they only show on long hovers (unlike tooltips). I do think it's a pattern we need to keep up with: If we use the euiTextTruncate
mixin, we must also apply the title attribute.
251266b
to
c18f2f0
Compare
c18f2f0
to
903fc28
Compare
{numFiltersDefined && | ||
<EuiNotificationBadge | ||
className="euiFilterButton__notification" | ||
size="m" | ||
aria-label={`${numActiveFilters || numFilters} ${!hasActiveFilters ? 'available' : 'active'} filters`} |
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.
@chandlerprall Can you help me set this up for localization? There's a few ifs in there that I'm not sure how to deal with.
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.
Process: the aria-label
value needs to be computed by EuiI18n
, so computing the value will be done by a function passed to Eui18n
's default prop. Any variables needed to compute the label are passed via values. Eui18n
's single child is a render prop (function called to render the actual child content) and is given the computed label as an argument.
{numFiltersDefined &&
<EuiI18n
token="euiFilterButton.filterBadge"
values={{ count: numActiveFilters || numFilters, hasActiveFilters }}
default={({ count, hasActiveFilters }) => `${count} ${hasActiveFilters ? 'active' : 'available'} filters`}
>
{
filterBadge => (
<EuiNotificationBadge
className="euiFilterButton__notification"
size="m"
aria-label={filterBadge}
color={isDisabled || !hasActiveFilters ? 'subdued' : 'accent'}
>
{numActiveFilters || numFilters}
</EuiNotificationBadge>
)
}
</EuiI18n>
}
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.
Thx!
src/components/filter_group/__snapshots__/filter_button.test.js.snap
Outdated
Show resolved
Hide resolved
@snide Can I get a final pass on this? |
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.
Code/TypeScript changes LGTM
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.
Ran locally. Rechecked the code after last feedback. LGTM.
…including the search bar
878346e
to
83d4585
Compare
Fixes #1818
Changed up a bit of the styles to allow for better responsiveness of EuiFilterGroup. It required adding an optional prop of
fullWidth
for better control.…including the search bar.
Deprecations
I have deprecated the use of
noDivider
onEuiFilterButton
s in favor of a more aptly named prop ofwithNext
in case it's more than just a divider change.Checklist
[ ] This was checked against keyboard-only and screenreader scenarios[ ] This required updates to Framer X components