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

Align Filter and Facet button styles #1589

Merged
merged 16 commits into from
Feb 26, 2019
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Feb 22, 2019

Fixes #1511

Essentially, this PR aligns the EuiFilterButton styles too look similar to the EuiFacetButton. It also means allowing to pass the "total available filters" when none are selected:

As well as highlight when there are selected filters:

Which meant adding numActiveFilters to EuiFilterButton.

As a reference, this is what the facet buttons look like


Typescript

I had originally attempted to convert them to TS, but because they each imported a non-TS'd component or the props of one, I couldn't do it. Maybe someone with better TS chops than me can. But I did make sure the TS def's were up to date so it will be easier to convert down the line.


Other

Added some extra props to EuiNotificationBadge:

  • size: s, m (default: s)
  • color: accent, subdued (default: accent)

I also did some quick updates to EuiAvatar (thicker font weight and font size adjustment) and to EuiIcon so that both do never shrink because of flex.

I also added a global mixin for the text-weight-shifting stuff since we're seeing it more often.


Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • [ ] Documentation examples were added
  • 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
  • [ ] This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Reviewed code and loaded locally. Do you think we should start using the active filters now in the default searchbar / table usage. Example here.

image

src/components/filter_group/filter_button.js Show resolved Hide resolved
src/global_styling/mixins/_typography.scss Show resolved Hide resolved
@ryankeairns
Copy link
Contributor

ryankeairns commented Feb 22, 2019

What do you think about using an underline to differentiate the active state, as opposed to a heavier weight, so that the line length doesn't grow?

screenshot 2019-02-22 14 49 11

@cchaos
Copy link
Contributor Author

cchaos commented Feb 22, 2019

Do you think we should start using the active filters now in the default searchbar / table usage.

Thought about it, looked at it, abandoned it. Someone more engineery would need to do that bit.

What do you think about using an underline to differentiate the active state

I think the bold weight is a lot easier to notice than an underline and just by way of consistency elsewhere we use underlines to indicate focus state not selected state.

@cchaos cchaos requested review from snide and ryankeairns February 22, 2019 22:19
src/components/facet/_facet_button.scss Outdated Show resolved Hide resolved
src/components/filter_group/index.d.ts Show resolved Hide resolved
src/components/filter_group/index.d.ts Outdated Show resolved Hide resolved
@cchaos cchaos requested a review from thompsongl February 25, 2019 15:57
@cchaos
Copy link
Contributor Author

cchaos commented Feb 25, 2019

Ready for another pass @thompsongl

CHANGELOG.md Outdated Show resolved Hide resolved
src/components/facet/index.d.ts Outdated Show resolved Hide resolved
src/components/facet/index.d.ts Outdated Show resolved Hide resolved
src/components/filter_group/index.d.ts Outdated Show resolved Hide resolved
src/components/filter_group/index.d.ts Show resolved Hide resolved
@cchaos cchaos requested a review from thompsongl February 25, 2019 20:42
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

🔜

src/components/facet/index.d.ts Outdated Show resolved Hide resolved
src/components/facet/index.d.ts Outdated Show resolved Hide resolved
src/components/filter_group/index.d.ts Show resolved Hide resolved
@cchaos cchaos requested a review from thompsongl February 26, 2019 02:05
@cchaos cchaos merged commit e6e3168 into elastic:master Feb 26, 2019
@cchaos cchaos deleted the filter-buttons branch February 26, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make EuiFilterButton and EuiFacetButton behave the same
4 participants