-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] EUI-fication of the filters #48452
Conversation
e0e51a8
to
d6fc63f
Compare
💔 Build Failed |
💔 Build Failed |
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.
Great that you already applied to Lens style, tested in Chrome, works 👍. Added comments about details
src/legacy/core_plugins/kibana/public/discover/components/field_chooser/field_selector.tsx
Outdated
Show resolved
Hide resolved
...egacy/core_plugins/kibana/public/discover/components/field_chooser/discover_field_search.tsx
Outdated
Show resolved
Hide resolved
.../core_plugins/kibana/public/discover/components/field_chooser/discover_field_search.test.tsx
Show resolved
Hide resolved
...egacy/core_plugins/kibana/public/discover/components/field_chooser/discover_field_search.tsx
Outdated
Show resolved
Hide resolved
...egacy/core_plugins/kibana/public/discover/components/field_chooser/discover_field_search.tsx
Outdated
Show resolved
Hide resolved
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
@elasticmachine merge upstream |
💚 Build Succeeded |
Nice! I'll take a look at that the design side as it appears there might be a panel inside of the popover menu. |
use compressed style inputs
@ryankeairns thanks for the comment, I merged your PR. regarding button group over select: is this how it is expected to look like then? |
💚 Build Succeeded |
@majagrubic that looks great, thanks for fitting this in! |
💚 Build Succeeded |
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.
@majagrubic checked this out locally and it looks great, thank you!
How much work would it be to have the popover close when clicking outside of it? Currently, it is only possible to close it by clicking on the filter button again. Clicking outside to close a popover is common pattern we've used elsewhere, see Lens as an example.
💚 Build Succeeded |
@ryankeairns mind if I do that in the follow-up PR? This one is already growing quite big. |
@majagrubic yep, that works for me, thanks! |
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.
Added one structural change request, think it makes sense to extract one sub component in it's own file. Update discussed this, and decided to keep it this way.
...egacy/core_plugins/kibana/public/discover/components/field_chooser/discover_field_search.tsx
Show resolved
Hide resolved
...egacy/core_plugins/kibana/public/discover/components/field_chooser/discover_field_search.tsx
Outdated
Show resolved
Hide resolved
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.
Thank a lot for another important step in redesigning Discover's field selector 👍 . Code LGTM, tested locally in Chrome
💚 Build Succeeded |
* New filter UI * Prototype of functionality * EUIFication of filters in Discover * Adding tests for field_selector component * Replacing FieldSelector with EuiForm components * Adding more discover field search tests * Removing console statement * Fixing failing functional test * Removing obsolete snapshot * design tweaks for filter popover * use compressed style inputs * Changing selectors to be EuiButtonGroup * Removing unnecessary if statement * Getting rid of ts-ignore warning
* New filter UI * Prototype of functionality * EUIFication of filters in Discover * Adding tests for field_selector component * Replacing FieldSelector with EuiForm components * Adding more discover field search tests * Removing console statement * Fixing failing functional test * Removing obsolete snapshot * design tweaks for filter popover * use compressed style inputs * Changing selectors to be EuiButtonGroup * Removing unnecessary if statement * Getting rid of ts-ignore warning
* New filter UI * Prototype of functionality * EUIFication of filters in Discover * Adding tests for field_selector component * Replacing FieldSelector with EuiForm components * Adding more discover field search tests * Removing console statement * Fixing failing functional test * Removing obsolete snapshot * design tweaks for filter popover * use compressed style inputs * Changing selectors to be EuiButtonGroup * Removing unnecessary if statement * Getting rid of ts-ignore warning
Summary
Related:
#47357
Filters selectors are now EUI components:
The logic is still tied to Angular. Need to add tests, so opening this as a draft.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers