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

[ML] Fix the browser's history navigation #80902

Merged
merged 5 commits into from
Oct 19, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Oct 16, 2020

Summary

Fixes regressions introduced by several PRs during 7.10, where it was no longer possible to return to the previous page in the browser's history by using the Back button.

Some highlights:

  1. We've been setting incorrect values for refresh intervals, e.g. combining value: 0 with pause:false. It was causing invoking the callback in a loop in some cases.
  2. Using a local state for refresh interval in DatePickerWrapper introduced collision of states and led to loop updates
  3. Subscriptions to timefilter service were producing some unnecessary states update

Related #73261.

Checklist

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895
Copy link
Member

qn895 commented Oct 19, 2020

Noticed after updating the refresh interval, the back button doesn't work any more... The rest seems to work pretty well!

@darnautov
Copy link
Contributor Author

good spot @qn895! I changed the logic to use the global state as a source of truth for refresh interval, check 0f40710

@darnautov darnautov added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Oct 19, 2020
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested (Chrome and FF) and LGTM.

At most it takes me two Back button clicks to get back to the Jobs List or Overview page from the Anomaly Explorer or Single Metric Viewer.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
ml 7.2MB 7.2MB -282.0B

History

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

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Great finds and cleanup!! Code and local test LGTM.

@darnautov darnautov merged commit 3d4f748 into elastic:master Oct 19, 2020
@darnautov darnautov deleted the ML-73261-fix-browser-history branch October 19, 2020 12:29
darnautov added a commit to darnautov/kibana that referenced this pull request Oct 19, 2020
* [ML] fix data picker and refresh state

* [ML] fix default pause value

* [ML] fix jest test

* [ML] global state as a source of truth for refreshInterval
darnautov added a commit that referenced this pull request Oct 19, 2020
* [ML] fix data picker and refresh state

* [ML] fix default pause value

* [ML] fix jest test

* [ML] global state as a source of truth for refreshInterval
darnautov added a commit that referenced this pull request Oct 19, 2020
* [ML] fix data picker and refresh state

* [ML] fix default pause value

* [ML] fix jest test

* [ML] global state as a source of truth for refreshInterval
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants