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

[EuiSuperDatePicker] Invoke onRefresh instead of onTimeChanged when clicking “Refresh” #1745

Conversation

sorenlouv
Copy link
Member

Follow-up to #1577

Since there is now an onRefresh callback, the update button should no longer call onTimeChanged when an actual refresh was performed (which doesn't change the time).
The ui of the button already distinguishes between the two states (update time range vs. manual refresh).
This is also the same callback that will be called, when the automatic refresh interval is fired.

refresh-vs-update

This is important because the consumer might want to have separate logic for when the time range was changed vs. when a refresh was invoked.

To make this backwards compatible this change only take effect for consumers who have implemented the onRefresh callback (searching through Kibana this will only affect APM)

@sorenlouv sorenlouv requested a review from nreese March 20, 2019 09:37
@sorenlouv sorenlouv changed the title [EuiSuperDatePicker] Invoke onRefresh when clicking “Refresh” [EuiSuperDatePicker] Invoke onRefresh instead of onTimeChanged when clicking “Refresh” Mar 20, 2019
@jasonrhodes
Copy link
Member

FYI don't forget the changelog entry :)

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Needs a changelog entry, but changes LGTM

@sorenlouv sorenlouv merged commit 3ae9cd4 into elastic:master Mar 20, 2019
@sorenlouv sorenlouv deleted the call-on-refresh-handler-when-clicking-refresh-button branch March 20, 2019 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants