[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display #6900

thomheymann · 2023-07-03T16:56:12Z

Summary

Exposes the existing EuiSearchFilters component which is useful when creating tables with custom filter bars.

Example: Alerting rule filters

An example of this is the Alerting Rules management screen in Kibana which currently had to re-implement filtering from scratch using custom components and logic to build the desired UI.

Example: API key filters

We are currently working on improving the API key management screen which would also benefit from having this component available. The display options are currently too restrictive when using the EuiSearchBar component directly.

For example it is not possible to group filters (adding a spacer) since all filters are rendered inside the same EuiFilterGroup component. Being able to render EuiSearchFilters directly would allow to render separate filter groups.

Currently possible

Desired

Note 1

This PR also fixes a rendering issue with the EuiFilterButton component where a minimum width is applied causing the button to expand unnecessarily:

Note 2

It is currently not possible to add numFilters prop using the EuiSearchBar so the only way to render the number of filters is to create a custom component. It would be nice if the field_value_selection filter type would support setting this.

src/components/filter_group/_filter_group.scss

kibanamachine · 2023-07-03T17:19:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6900/

…rrow

cee-chen · 2023-07-03T17:44:03Z

src/components/search_bar/index.ts

@@ -12,5 +12,6 @@ export type {
  QueryType,
 } from './search_bar';
 export { EuiSearchBar, Query, Ast } from './search_bar';
+export { EuiSearchFilters } from './search_filters';


@thomheymann I think my minor hesitation with this change is one of general high-level consistency, rather than any strict objection to this specific scenario.

We already allow consumers to import internal components/types if needed by reaching directly into our lib/components, e.g.

import { EuiSearchFilters } from '@elastic/eui/lib/components/search_bar/search_filters';

While it's not officially documented, we do at times suggest it as a workaround for consumers with specific edge use cases or non-default usages.

With that in mind, I think my concerns are:

What warrants this as top-level import vs reaching into EUI internals?

If we are going to export EuiSearchBar internals, Why not export EuiSearchBox as well EuiSearchFilters?

I also have a slight hesitation that it's not clear that EuiSearchFilters belongs to EuiSearchBar (as opposed to it being named EuiSearchBarFilters, which is a naming schema that other components with sub-components tend to follow). I'm worried that consumers will see it autofill at the top level and try to use it with no clear documentation.

Trying to put that all together, I think my preference for a more consistent architecture and developer experience would be one of the following:

As a consumer, import EuiSearchFilters directly from its component file instead

As EUI, modify EuiSearchBar to support the setup screenshotted/mocked in your PR description instead of custom internals

As EUI, export all internal subcomponents of EuiSearchBar, and update their names to make it clearer that they are subcomponents (i.e., EuiSearchFilters->EuiSearchBarFilters, EuiSearchBox->EuiSearchBarBox)
(Although this is still a bit tricky because there are a ton of very esql-specific subcomponents in the query/ and filters/ subcomponents 🤔)

I have tried doing this at first but it's not possible to import the component directly since the types are missing:

Could not find a declaration file for module '@elastic/eui/lib/components/search_bar/search_filters'. '/kibana/node_modules/@elastic/eui/lib/components/search_bar/search_filters.js' implicitly has an 'any' type.

It's also considered bad practice since any internal change (e.g. moving files/folders around inside EUI) would break these direct imports.

In regards to your hesitation to make this a top-level export, I get where you're coming from but you could argue the same about EuiSearchBar which is also exposed separately even though it is rendered by EuiTable and I don't think it's used anywhere without also rendering a table.

If EUI would be able to handle the described use cases directly that would obviously be my preference but I worry that getting the necessary changes prioritised and built will take a long time.

but it's not possible to import the component directly since the types are missing

Ahh gotcha, good call on that 🤔

I get where you're coming from but you could argue the same about EuiSearchBar which is also exposed separately even though it is rendered by EuiTable and I don't think it's used anywhere without also rendering a table.

For context here, I'd say the major difference is documentation - we document EuiSearchBar thoroughly as a standalone component. If something is available as a top-level export, we'd typically want to add documentation/examples of how to use it standalone.

Also worth keeping in mind that while Kibana is definitely our main consumer, it's not our only consumer.

Totally appreciate your note about expedience - let's compromise with exporting the internal search filters ~~and search box~~, but rename them so that it's extremely clear just from the name alone that they're sub-components of EuiSearchBar.

edit: changing my mind on exporting the search box as well. TBH, it's a little hard for me to reconcile exporting this separately for use outside of EuiSearchBar and next to EuiFilterGroup. Renaming it will at least namespace it a bit better, but I'd strongly prefer to extend EuiSearchBar to support your desired outcome, if you don't mind opening a separate issue for that sometime

kibanamachine · 2023-07-03T17:54:20Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6900/

thomheymann · 2023-07-05T08:01:23Z

@cee-chen What's the best way of progressing this?

…opdown arrow" This reverts commit d594861.

- to make it clearer that the new export is intended to be a subcomponent of `EuiSearchBar`

cee-chen · 2023-07-06T03:41:18Z

Apologies for the delay @thomheymann - the mid-week US holiday has got me off all-kilter. I've reverted my original feedback commit and pushed up a new one with a namespaced rename for EuiSearchBarFilters (so the subcomponent relationship is a little clearer to devs who make copious use of autofill, aka me :). I'm good to merge this PR once CI passes!

It's still a little hard for me to mentally reconcile the use of the search bar's filter components outside the search bar. If you have time, I'd super appreciate a new issue describing the type of customization / grouping API you'd want to use to get EuiSearchBar behaving like your depicted screenshot. We can hopefully prioritize it and maybe remove the export down the road at that point 🤞

kibanamachine · 2023-07-06T04:00:45Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6900/

thomheymann · 2023-07-06T08:44:10Z

Brilliant, thanks so much for your help on this!

I'll put together a follow up issue.

## Summary `eui@83.1.0` ⏩ `eui@84.0.0` --- ## [`84.0.0`](https://github.com/elastic/eui/tree/v84.0.0) - Updated `EuiDualRange`'s `minInputProps` and `maxInputProps` to support passing more props to underlying inputs ([#6902](elastic/eui#6902)) - `EuiFocusTrap` now supports configuring cross-iframe focus trapping via the `crossFrame` prop ([#6908](elastic/eui#6908)) **Bug fixes** - Fixed `EuiFilterButton` icon display ([#6900](elastic/eui#6900)) - Fixed `EuiCombobox` compressed plain text display ([#6910](elastic/eui#6910)) - Fixed visual appearance of collapse buttons on collapsible `EuiResizablePanel`s ([#6926](elastic/eui#6926)) **Breaking changes** - `EuiFocusTrap` now defaults to *not* trapping focus across iframes ([#6908](elastic/eui#6908)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

thomheymann · 2023-07-15T10:36:50Z

@cee-chen I've created the follow up issue here #6952

thomheymann added 2 commits July 3, 2023 17:24

Expose EuiSearchFilters component

f319d8f

Fix styling for filter buttons with icon

6c14536

thomheymann requested a review from cee-chen July 3, 2023 17:05

cee-chen changed the title ~~Expose search filters~~ [EuiSearchBar] Export EuiSearchFilters; [EuiFilterGroup] Fix icon display Jul 3, 2023

cee-chen reviewed Jul 3, 2023

View reviewed changes

src/components/filter_group/_filter_group.scss Show resolved Hide resolved

Tweak opinionated icon styling to only affect the intended dropdown a…

d594861

…rrow

cee-chen changed the title ~~[EuiSearchBar] Export EuiSearchFilters; [EuiFilterGroup] Fix icon display~~ [EuiSearchBar] Export EuiSearchFilters; [EuiFilterGroup] Fix icon display for non-dropdown buttons Jul 3, 2023

cee-chen reviewed Jul 3, 2023

View reviewed changes

cee-chen added 2 commits July 5, 2023 20:23

Revert "Tweak opinionated icon styling to only affect the intended dr…

8553b76

…opdown arrow" This reverts commit d594861.

[PR feedback] Namespacing

d52a458

- to make it clearer that the new export is intended to be a subcomponent of `EuiSearchBar`

cee-chen changed the title ~~[EuiSearchBar] Export EuiSearchFilters; [EuiFilterGroup] Fix icon display for non-dropdown buttons~~ [EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterGroup] Fix icon display for non-dropdown buttons Jul 6, 2023

cee-chen changed the title ~~[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterGroup] Fix icon display for non-dropdown buttons~~ [EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display Jul 6, 2023

chagnelog

6b9dc0b

cee-chen approved these changes Jul 6, 2023 •

edited

Loading

View reviewed changes

thomheymann merged commit 6774163 into elastic:main Jul 6, 2023
1 check passed

thomheymann mentioned this pull request Jul 6, 2023

Allow API Keys to be managed for remote clusters elastic/kibana#142400

Closed

cee-chen mentioned this pull request Jul 11, 2023

Upgrade EUI to v84.0.0 elastic/kibana#161716

Merged

thomheymann mentioned this pull request Jul 15, 2023

[EuiTable] [EuiSearchBar] Filter improvements #6952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display #6900

[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display #6900

thomheymann commented Jul 3, 2023 •

edited

Loading

kibanamachine commented Jul 3, 2023

cee-chen Jul 3, 2023 •

edited

Loading

thomheymann Jul 3, 2023

cee-chen Jul 6, 2023 •

edited

Loading

kibanamachine commented Jul 3, 2023

thomheymann commented Jul 5, 2023

cee-chen commented Jul 6, 2023 •

edited

Loading

kibanamachine commented Jul 6, 2023

thomheymann commented Jul 6, 2023

thomheymann commented Jul 15, 2023

[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display #6900

[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display #6900

Conversation

thomheymann commented Jul 3, 2023 • edited Loading

Summary

Example: Alerting rule filters

Example: API key filters

Currently possible

Desired

Note 1

Note 2

kibanamachine commented Jul 3, 2023

cee-chen Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

thomheymann Jul 3, 2023

Choose a reason for hiding this comment

cee-chen Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Jul 3, 2023

thomheymann commented Jul 5, 2023

cee-chen commented Jul 6, 2023 • edited Loading

kibanamachine commented Jul 6, 2023

thomheymann commented Jul 6, 2023

thomheymann commented Jul 15, 2023

thomheymann commented Jul 3, 2023 •

edited

Loading

cee-chen Jul 3, 2023 •

edited

Loading

cee-chen Jul 6, 2023 •

edited

Loading

cee-chen commented Jul 6, 2023 •

edited

Loading