Skip to content
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

[EuiFilterButton] Allow for zero notifications #1510

Merged
merged 3 commits into from
Feb 5, 2019

Conversation

pickypg
Copy link
Member

@pickypg pickypg commented Feb 2, 2019

Summary

The existing check implicitly disallows 0, which causes the rendered 0 to lack the proper styling.

image

versus with this fix

image

Checklist

  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios

Workaround

To anyone using it prior to this fix, you could supply numFilters as numFilters={value || undefined} to avoid the awkward 0.

@pickypg pickypg added the bug label Feb 2, 2019
@cchaos
Copy link
Contributor

cchaos commented Feb 3, 2019

The numFilters != null check should probably be checked and stored early on in the component, because there are classes applied that rely on that logic as well

{ 'euiFilterButton__text-hasNotification': numFilters, },

The existing check implicitly disallows `0`, which causes the
rendered `0` to lack the proper styling.
@pickypg pickypg force-pushed the fix/allow-zero-notifications branch from 87f54b5 to d0d2cc0 Compare February 4, 2019 22:22
@pickypg
Copy link
Member Author

pickypg commented Feb 4, 2019

Good catch @cchaos! Updated to share the boolean check across both usages.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pickypg pickypg merged commit 2b48b41 into elastic:master Feb 5, 2019
@pickypg pickypg deleted the fix/allow-zero-notifications branch February 5, 2019 00:25
@snide snide mentioned this pull request Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants