-
Notifications
You must be signed in to change notification settings - Fork 837
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
[EuiSearchBar] Export EuiSearchBarFilters; [EuiFilterButton] Fix icon display #6900
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6900/ |
src/components/search_bar/index.ts
Outdated
@@ -12,5 +12,6 @@ export type { | |||
QueryType, | |||
} from './search_bar'; | |||
export { EuiSearchBar, Query, Ast } from './search_bar'; | |||
export { EuiSearchFilters } from './search_filters'; |
There was a problem hiding this comment.
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:
- What warrants this as top-level import vs reaching into EUI internals?
- If we are going to export
EuiSearchBar
internals, Why not exportEuiSearchBox
as wellEuiSearchFilters
? - I also have a slight hesitation that it's not clear that
EuiSearchFilters
belongs 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
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 thequery/
andfilters/
subcomponents 🤔)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 byEuiTable
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
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`
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>
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 renderEuiSearchFilters
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 theEuiSearchBar
so the only way to render the number of filters is to create a custom component. It would be nice if thefield_value_selection
filter type would support setting this.