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.

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

[Infra and Logs UI] Fixes "sticky filter" problem #40226

Merged
merged 2 commits into from
Jul 3, 2019

Conversation

jasonrhodes
Copy link
Member

@jasonrhodes jasonrhodes commented Jul 3, 2019

Closes #39991

Summary

The problem in this issue is caused by the fact that our autocomplete query bar only submits a value on enter. If you click out of the field without pressing "enter" first, your newest filter value won't be applied. This creates unexpected scenarios, especially when trying to delete existing filters (and especially when trying to delete existing filters that returned no results).

This PR fixes this issue in two ways.

First, if you have deleted all of the existing filters, the data will be re-queried automatically even before you exit the field or press enter. (This gives you your unfiltered data back as quickly as possible so you can begin to search again.)

one-metric-on-delete

Second, if you have more than one filter, the query will at least re-execute if you delete the "bad" filter and then exit the input, i.e. you click somewhere else rather than pressing "enter".

two-metric-on-blur

@jasonrhodes jasonrhodes requested a review from a team as a code owner July 3, 2019 01:59
@jasonrhodes jasonrhodes force-pushed the 39991-fix-sticky-filters branch from 29d44eb to e09f75a Compare July 3, 2019 02:01
@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

Ping @tbragin / @sorantis for your reference, see GIF screenshots above.

@jasonrhodes
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

As I think about this more, I think maybe a better/more complete solution (down the road) is a debounced "as you type" submit, since we don't surface any kind of submit or "apply changes" button.

@jasonrhodes
Copy link
Member Author

retest

@Kerry350 Kerry350 self-requested a review July 3, 2019 10:30
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

filter-bar

@jasonrhodes jasonrhodes added the Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services label Jul 3, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@@ -156,6 +157,12 @@ export class AutocompleteField extends React.Component<
case 'End':
this.updateSuggestions();
break;
case 'Backspace':
// if the user has just deleted everything, submit to reset
if (this.props.value === '') {
Copy link
Member

Choose a reason for hiding this comment

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

How about we move this to either the changeValue method or even out of this component into the onChange handler passed in? Then it wouldn't work for just the Backspace key but also for delete or any other means of emptying the input.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Moving to changeValue presented some race condition stuff where the changed state may not be updated and this.submit() only uses the value from this.props, but we are already checking for the existence of a new value prop in componentDidUpdate so I moved it there and verified it all still works as expected (but now it also works if you "ctrl-x" cut, for example).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

retest

@jasonrhodes jasonrhodes force-pushed the 39991-fix-sticky-filters branch from c15695d to 9792de9 Compare July 3, 2019 16:09
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

I haven't tested the whole thing thoroughly but the new empty value check location looks better 👍

@jasonrhodes jasonrhodes merged commit 0049345 into elastic:master Jul 3, 2019
@jasonrhodes jasonrhodes deleted the 39991-fix-sticky-filters branch July 3, 2019 17:21
jasonrhodes added a commit to jasonrhodes/kibana that referenced this pull request Jul 3, 2019
* Submits query bar form when everything has been deleted and also on blur to prevent sticky value problem

* Moved reset to cDU to cover all deleted value cases, not just Backspace
jasonrhodes added a commit to jasonrhodes/kibana that referenced this pull request Jul 3, 2019
* Submits query bar form when everything has been deleted and also on blur to prevent sticky value problem

* Moved reset to cDU to cover all deleted value cases, not just Backspace
jasonrhodes added a commit that referenced this pull request Jul 3, 2019
* Submits query bar form when everything has been deleted and also on blur to prevent sticky value problem

* Moved reset to cDU to cover all deleted value cases, not just Backspace
@tbragin
Copy link
Contributor

tbragin commented Jul 4, 2019

Thank you @jasonrhodes and @weltenwort !

jasonrhodes added a commit that referenced this pull request Jul 22, 2019
* Submits query bar form when everything has been deleted and also on blur to prevent sticky value problem

* Moved reset to cDU to cover all deleted value cases, not just Backspace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra UI] Clearing Filters field doesn't auto-reset the view
5 participants