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

[Logs UI] Improve log threshold rule field selection #111135

Merged
merged 3 commits into from Sep 7, 2021
Merged

[Logs UI] Improve log threshold rule field selection #111135

merged 3 commits into from Sep 7, 2021

Conversation

miltonhultgren
Copy link
Contributor

@miltonhultgren miltonhultgren commented Sep 3, 2021 •
edited

Summary

Fixes #110611

The use of select for the field when creating a Log Treshold rule made it harder to find and select the desired field in a dataset with a lot of fields. This PR makes the expression use a comboBox instead to allow users to search for the field they want while still seeing some suggestions.

Kapture 2021-09-03 at 15 22 30

Checklist

For maintainers

@miltonhultgren miltonhultgren added auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.0 v8.0.0 labels Sep 3, 2021
@miltonhultgren miltonhultgren changed the title [Logs UI] Use ComboBox for criterion field selection [Logs UI] Improve log threshold rule field selection Sep 6, 2021
@miltonhultgren miltonhultgren marked this pull request as ready for review September 6, 2021 11:31
@miltonhultgren miltonhultgren requested a review from a team as a code owner September 6, 2021 11:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@miltonhultgren
Copy link
Contributor Author

@elasticmachine merge upstream

@weltenwort weltenwort self-requested a review September 6, 2021 18:45
weltenwort
weltenwort requested changes Sep 6, 2021
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Works pretty well, nice touch to close the whole popover on selection. 👏

I found just one edge case which I commented on below.

x-pack/plugins/infra/public/alerting/log_threshold/components/expression_editor/criterion.tsx
(e) => {
const fieldName = e.target.value;
([selectedOption]) => {
const fieldName = selectedOption.label;
Copy link
Member

@weltenwort weltenwort Sep 6, 2021 •
edited

Choose a reason for hiding this comment

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

When I press backspace in the input field of the combobox this seems to be called with selectedOption being undefined because the array is empty for one call. Can we handle that case somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find! I added a guard that checks if there is a selectedOption, and if not I update the criterion to empty which results in a form validation error. I keep the popover open so the user gets a hint that they have to select something.

Kapture 2021-09-07 at 08 51 08

@miltonhultgren miltonhultgren force-pushed the 110611-log-threshold-searchable-field branch from 81344c1 to 004ffb6 Compare September 7, 2021 06:48
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.7MB 1.7MB +113.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

weltenwort
weltenwort approved these changes Sep 7, 2021
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

nicely done 👏

@miltonhultgren miltonhultgren merged commit 51fd4ab into elastic:master Sep 7, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 7, 2021
@miltonhultgren @kibanamachine
[Logs UI] Improve log threshold rule field selection (elastic#111135)
855378f 
* [Logs UI] Use ComboBox for criterion field selection (elastic#110996 
* [Logs UI] Make log threshold criterion field clearable
@kibanamachine kibanamachine mentioned this pull request Sep 7, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 8, 2021
@kibanamachine @miltonhultgren
[Logs UI] Improve log threshold rule field selection (#111135) (#111372)
3ceb06e 
* [Logs UI] Use ComboBox for criterion field selection (#110996 
* [Logs UI] Make log threshold criterion field clearable

Co-authored-by: Milton Hultgren <milton.hultgren@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weltenwort weltenwort weltenwort approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Logs UI] Selecting field in Rule creation is hard when data contains lots of fields
4 participants
@miltonhultgren @elasticmachine @kibanamachine @weltenwort