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

[Input Controls] Fix Resize Resetting Selections #76573

Merged
merged 9 commits into from
Sep 10, 2020

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Sep 2, 2020

This PR fixes #76142.

Overview

The problem seems to stem from render() getting called on resize in src/plugins/visualizations/public/components/visualization_chart.tsx, whereas the input controls visualization seems to assume that render() should fully reset all the controls, and should only be called on startup. src/plugins/input_control_vis/public/vis_controller.tsx

In my research it seems like this problem has been around since 7.7.

The fix is to use a flag to ensure that the initialization only happens once, and subsequent re-renders don't fully re-initialize.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ThomThomson ThomThomson changed the title [DRAFT] Workaround for controls reset [Input Controls] Fix Resize Resetting Selections Sep 3, 2020
@ThomThomson ThomThomson marked this pull request as ready for review September 4, 2020 14:34
@ThomThomson ThomThomson requested a review from a team September 4, 2020 14:34
@ThomThomson ThomThomson added v7.9.2 and removed v7.9.1 labels Sep 4, 2020
Copy link
Contributor Author

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Open question:
Usage of isEqual here may be inefficient and better replaced with a custom diff.

if (!this.isLoaded || !isEqual(visParams, this.visParams)) {

It looks like the items that need to be comapred are:
pinFilters, updateFIltersOnChange, useTimeFilter, and the values inside the controls array, which have many keys that could have changed.

Due to the complexity of the diff required, I figure that isEqual might be the right choice.

Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

Tested locally and works well, didn't find any regression.
Left some comments to discuss before merging.

Comment on lines +57 to +63
this.timeFilterSubscription = deps.data.query.timefilter.timefilter
.getTimeUpdate$()
.subscribe(() => {
if (this.visParams?.useTimeFilter) {
this.isLoaded = false;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure do you really need the subscription?
I didn't find the case where do you need to handle this specific parameter.
Could you please clarify this approach? Thnx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This subscription is used to re-initialize the input controls when the 'useTimeFilter' setting is on, and the time filter has changed. This is an expected behaviour of input controls, and was missing after I stopped the 're-initialize everything on each render' behavior.

src/plugins/input_control_vis/public/vis_controller.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

LGTM!
Checked locally, everything works as expected.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Tested locally in Chrome on Mac OS, works as described. I am not too familiar with input controls code, but the changes seem reasonable to me.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
inputControlVis 296.2KB +605.0B 295.6KB

History

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

@ThomThomson ThomThomson merged commit 5d12eda into elastic:master Sep 10, 2020
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Sep 10, 2020
Fixed resizing controls visualization resetting selections
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Sep 10, 2020
Fixed resizing controls visualization resetting selections
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Sep 10, 2020
Fixed resizing controls visualization resetting selections
ThomThomson added a commit that referenced this pull request Sep 10, 2020
Fixed resizing controls visualization resetting selections
ThomThomson added a commit that referenced this pull request Sep 10, 2020
Fixed resizing controls visualization resetting selections
ThomThomson added a commit that referenced this pull request Sep 10, 2020
Fixed resizing controls visualization resetting selections
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control visualizations resetting when multi-select drop down reaches the bottom of the visualization
4 participants