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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jul 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f319d8f
Expose EuiSearchFilters component
thomheymann Jul 3, 2023
6c14536
Fix styling for filter buttons with icon
thomheymann Jul 3, 2023
d594861
Tweak opinionated icon styling to only affect the intended dropdown a…
cee-chen Jul 3, 2023
8553b76
Revert "Tweak opinionated icon styling to only affect the intended dr…
cee-chen Jul 6, 2023
d52a458
[PR feedback] Namespacing
cee-chen Jul 6, 2023
6b9dc0b
chagnelog
cee-chen Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/components/filter_group/_filter_group.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@
min-width: $euiSize * 6;
}

> .euiFilterButton--hasIcon {
min-width: $euiSize * 8;
}

cee-chen marked this conversation as resolved.
Show resolved Hide resolved
// Force popover anchors to expand for now
.euiPopover__anchor {
display: block;
Expand Down
1 change: 1 addition & 0 deletions src/components/search_bar/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ export type {
QueryType,
} from './search_bar';
export { EuiSearchBar, Query, Ast } from './search_bar';
export { EuiSearchFilters } from './search_filters';
Copy link
Member

@cee-chen cee-chen Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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:

  1. What warrants this as top-level import vs reaching into EUI internals?
  2. If we are going to export EuiSearchBar internals, Why not export EuiSearchBox as well EuiSearchFilters?
  3. 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:

  1. As a consumer, import EuiSearchFilters directly from its component file instead
  2. As EUI, modify EuiSearchBar to support the setup screenshotted/mocked in your PR description instead of custom internals
  3. 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 🤔)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@cee-chen cee-chen Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

export type { SearchFilterConfig } from './search_filters';
export type { FieldValueOptionType } from './filters/field_value_selection_filter';