[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display#6900
Conversation
cee-chen
changed the title
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6900/ |
cee-chen
changed the title
src/components/search_bar/index.ts
Outdated
| QueryType, | ||
| } from './search_bar'; | ||
| export { EuiSearchBar, Query, Ast } from './search_bar'; | ||
| export { EuiSearchFilters } from './search_filters'; |
There was a problem hiding this comment.
@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
EuiSearchBarinternals, Why not exportEuiSearchBoxas wellEuiSearchFilters? - I also have a slight hesitation that it's not clear that
EuiSearchFiltersbelongs toEuiSearchBar(as opposed to it being namedEuiSearchBarFilters, 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
EuiSearchFiltersdirectly from its component file instead - As EUI, modify
EuiSearchBarto 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 thequery/andfilters/subcomponents 🤔)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
EuiSearchBarwhich is also exposed separately even though it is rendered byEuiTableand 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
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6900/ |
|
@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
changed the title
cee-chen
changed the title
|
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 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 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6900/ |
|
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>
3 participants
Summary
Exposes the existing
EuiSearchFilterscomponent 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
EuiSearchBarcomponent directly.For example it is not possible to group filters (adding a spacer) since all filters are rendered inside the same
EuiFilterGroupcomponent. Being able to renderEuiSearchFiltersdirectly would allow to render separate filter groups.Currently possible
Desired
Note 1
This PR also fixes a rendering issue with the
EuiFilterButtoncomponent where a minimum width is applied causing the button to expand unnecessarily:Note 2
It is currently not possible to add
numFiltersprop using theEuiSearchBarso the only way to render the number of filters is to create a custom component. It would be nice if thefield_value_selectionfilter type would support setting this.