Skip to content

Timepicker step forward/backward should not overlap - #11131

Merged
lukasolson merged 2 commits into
elastic:masterfrom
lukasolson:fix-time-stepping
Apr 11, 2017
Merged

lukasolson merged 2 commits into
elastic:masterfrom
lukasolson:fix-time-stepping

Conversation

@lukasolson

Copy link
Copy Markdown
Contributor

Fixes #11061.

Prior to this PR, if you used the step forward/backward buttons in the timepicker, the new range would include one millisecond from the previous range, which could result in documents showing up for both ranges.

For example, if I selected 2017-04-10T00:00:00.000Z-2017 to 04-10T23:59:59.999Z, then stepped backward, the new range would be 2017-04-09T00:00:00.001Z to 2017-04-10T00:00:00.000Z. Since the ranges are inclusive, documents that had the timestamp of exactly 2017-04-10T00:00:00.000Z-2017 would show up in both ranges.

This PR fixes the behavior by stepping forward/backward by an extra millisecond so that the ranges do not overlap.

This PR fixes the step forward/backward buttons in the timepicker so that the ranges do not overlap. Prior to this PR, if you stepped forward/backward, the new range would include one millisecond of time in both ranges, which could result in documents being visible in both time ranges.

@Bargs Bargs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Bargs Bargs removed their assignment Apr 10, 2017

@weltenwort weltenwort left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just left a small thought in the inline comment

return {
from: max.toISOString(),
to: moment(max).add(diff).toISOString(),
from: moment(max).add(1).toISOString(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about we add the unit 'ms' here (an in similar places) as in .add(1, 'ms') to avoid misunderstandings? The behavior of leaving out the unit is not documented even though it seems to work.

@lukasolson
lukasolson merged commit 6752dfe into elastic:master Apr 11, 2017
lukasolson added a commit that referenced this pull request Apr 11, 2017
* Timepicker step forward/backward should not overlap
This PR fixes the step forward/backward buttons in the timepicker so that the ranges do not overlap. Prior to this PR, if you stepped forward/backward, the new range would include one millisecond of time in both ranges, which could result in documents being visible in both time ranges.

* Add unit for add/subtract methods
lukasolson added a commit that referenced this pull request Apr 11, 2017
* Timepicker step forward/backward should not overlap
This PR fixes the step forward/backward buttons in the timepicker so that the ranges do not overlap. Prior to this PR, if you stepped forward/backward, the new range would include one millisecond of time in both ranges, which could result in documents being visible in both time ranges.

* Add unit for add/subtract methods
@lukasolson

Copy link
Copy Markdown
Contributor Author

Backported to 5.x (5.4.0) in 2530365.
Backported to 5.3 (5.3.1) in b7417c4.

@lukasolson
lukasolson deleted the fix-time-stepping branch March 27, 2018 21:08
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
* Timepicker step forward/backward should not overlap
This PR fixes the step forward/backward buttons in the timepicker so that the ranges do not overlap. Prior to this PR, if you stepped forward/backward, the new range would include one millisecond of time in both ranges, which could result in documents being visible in both time ranges.

* Add unit for add/subtract methods
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.

Timepicker step forward/backward buttons should not result in ranges that overlap

3 participants