Closes #2394 - Allow relative from and to #10990

simianhacker · 2017-03-31T23:26:54Z

Closes #2394.
Closes #6732.

This PR allows you to define a relative "to" string.

Before:

After:

simianhacker · 2017-03-31T23:41:31Z

Looks like there are some tests for the timepicker that need to change as well

lukasolson

Currently, the "quick" and "absolute" pickers don't allow you to select a "to" date before the "from" date. It'd be nice if the "relative" behavior followed this same pattern.

Also, I think it'd be nice to add the "set to now" buttons here like we have in the absolute picker, because there isn't a real easy way to get back to "now" other than manually setting the value inside the text input to 0. Thoughts?

simianhacker · 2017-04-03T22:46:22Z

Yeah... I was thinking about adding the "Set To Now", I figured just typing zero was pretty easy but people might not know to do that. Good point on the checks, I will add them.

simianhacker · 2017-04-03T23:23:29Z

Added "Set To Now":

And added a check:

lukasolson

Looks like there's a bug here. The date that shows up in the "To" field doesn't actually match what gets selected as the date when you use "Round to the nearest." For example, here I've got "6 months ago, rounded to the nearest month" selected as both "From" and "To".

In the timepicker, I think we assume that "rounding to the nearest" means rounding down for "From" and rounding up for "To".

If you wanted to add a test to make sure the text shows the correct date in this case, that'd be awesome.

lukasolson · 2017-04-04T00:03:17Z

src/ui/public/timepicker/timepicker.html

-                    <span ng-show="relative.preview">{{relative.preview}}</span>
-                    <span ng-hide="relative.preview"><i>Invalid Expression</i></span>
+                    <span ng-show="relative.from.preview">{{relative.from.preview}}</span>
+                    <a class="label label-default" ng-click="setRelativeToNow('from')">Set To Now</a>


As it currently stands, it doesn't make much sense to add the "Set to Now" button in the "From" field, because there's no way to have a "To" that's in the future (as far as the relative picker is concerned). However, since we just added it to the absolute timepicker, and since there's an open issue to add future relative dates, I'm fine with it.

So remove it?

simianhacker · 2017-04-04T11:52:33Z

It looks like I forgot to add the flag for rounding up for the "to" dateMath.parse() calls. I added it and that should fix the weirdness.

simianhacker · 2017-04-04T17:55:43Z

I'm going to remove the "review" tag from this and also work on closing #6732

lukasolson · 2017-04-04T17:59:27Z

Sounds good! Thanks for taking these on.

lukasolson · 2017-04-05T00:23:19Z

@cjcenizal Curious if you could provide your feedback about the error message when the "from" value is after the "to" value (see the screenshot in this comment).

cjcenizal · 2017-04-05T00:28:11Z

@lukasolson I would highlight both fields in red by setting them as invalid, and put the error message in red beneath the "Round the nearest minute" checkbox in the "From" panel. This way people see that there's an error, it's specific to their input, and they read the error message in the same context as the error's cause.

lukasolson

When I select one of the options in the drop-down for the future, the "round to the" label doesn't seem to adjust properly:

Also, I know I had mentioned removing the "set to now" button for the "from" field, but now that you've added support for future dates, and seeing as how the "set to now" button is in the absolute "from" as well, I think it makes sense to add it back.

simianhacker · 2017-04-05T13:21:54Z

@lukasolson Fixed the "round to the" label. The new value for the future units was messing up the lookup.

simianhacker · 2017-04-05T15:43:34Z

@lukasolson All the things

lukasolson

Sorry, found another thing. 😬 If I have "0 days" selected as the "to", then check "round to the day", it still says "now". Shouldn't that actually be the end of today? (If I go into the url and change the "to" value to "now-0d/d", it's a different date than simply "now".)

lukasolson

WOOT LGTM!

@tsullivan any concerns about these changes for monitoring, and @stacey-gammon any concerns about these changes for dashboard?

alexfrancoeur · 2017-04-06T10:45:04Z

The behavior for the back and forward buttons is a bit odd. I'd expect the relative time to keep the difference between from and to but it seems to shift randomly. To automatically reverts to 0 seconds and from seems to work as expected when traversing backwards but if you move forward in time the dropdown changes to seconds with an incorrect number.

Another minor thing I found. I noticed that if I set the from or to values to 0 [anything], the dropdown reverts back to seconds / now regardless of the selection I made. I'd expect the dropdown to persist the value I chose while still loading "now".

If I use the "round to" option, it will persist the selection in the drop down. Granted, the date value also changes here.

From a monitoring perspective (@bohyun-e @tsullivan) there is now a "from now" option (hours from now, months from now, etc.). It's its current state, this would make it that much easier to view future dates where there is no monitoring data.

stacey-gammon · 2017-04-06T14:39:03Z

I'm not sure what I did to cause this, but I am getting an error when trying to click the Relative button:

TypeError: Cannot read property 'match' of undefined
    at getRelativeString (timepicker.js:185)
    at Scope.$scope.formatRelative (timepicker.js:163)
    at Scope.$scope.setMode (timepicker.js:129)
    at fn (eval at compile (commons.bundle.js?v=8467:1), <anonymous>:4:229)
    at callback (angular.js:23549)
    at Scope.$eval (angular.js:15989)
    at Scope.$apply (angular.js:16089)
    at HTMLAnchorElement.<anonymous> (angular.js:23554)
    at HTMLAnchorElement.dispatch (jquery.js:4737)
    at HTMLAnchorElement.elemData.handle (jquery.js:4549)

Heres a screenshot of my current settings:

stacey-gammon · 2017-04-06T14:43:56Z

Re: the bug above, it seems to be specifically related to the time range, as going back to a different one then fixes the problem.

One more thing I notice - it doesn't seem to be updating the absolute section as I go back and forth:

Though it does once I close and re-open the time picker.

simianhacker · 2017-04-06T17:57:14Z

@stacey-gammon Changing the relative then going to absolute doesn't retain the new date (from relative) in master. This mimics the same behavior. I did fix the bug you found though with the exception above.

@alexfrancoeur I fixed the code so that when you have a 4 hour range (now-2h-now+2h) then start scrolling absolute will retain the range between. So if you go to the left once then it will change to (now-6h-now-2h). As for retaining the unit... If I change the code to retain the unit (now-0h) then instead of saying now to ~in 2 hours it will say ~a few seconds ago to ~in 2 hours. Where if the from time string is set to (now) it will actually say now.

I think we should merge this PR and then file a new issue to change the dateMath library to handle now-0h as 'now' instead of '~a few seconds ago` (which is an a different repo). Then we can properly fix it to the behavior you prefer above.

alexfrancoeur · 2017-04-06T19:40:19Z

@simianhacker ++ sounds good to me. Retaining the unit is a minor issue in my opinion.

stacey-gammon · 2017-04-06T20:03:24Z

Sorry @simianhacker I came in late to the PR, thought this had something to do with the previous/next buttons. You are right about the current functionality in master. LGTM! 👍

bohyun-e · 2017-04-06T21:18:22Z

@alexfrancoeur

From a monitoring perspective (@bohyun-e @tsullivan) there is now a "from now" option (hours from now, months from now, etc.). It's its current state, this would make it that much easier to view future dates where there is no monitoring data.

Not following.. could you clarify?

Bargs

I noticed when weeks is selected as the unit it gets turned into days in the timepicker. Not really an issue, but a little odd since the other units don't seem to get converted. Don't think it's anything to block on, but thought I'd mention it.

This second thing is just sort of a pet peeve of mine, but I hate when forms freak out on me during normal usage. When I delete the current input in order to type something else in, everything goes red for a second before I start typing which is jarring. Would it be worthwhile to make the error handling a bit smarter so it doesn't make everything red when a single input is empty?

Bargs · 2017-04-06T21:50:03Z

src/ui/public/timepicker/parse_relative_parts.js

+  let results = {};
+  const matches = _.isString(part) && part.match(/now(([\-\+])([0-9]+)([smhdwMy])(\/[smhdwMy])?)?/);
+
+  if (matches && !matches[1]) {


Just a suggestion, but some destructuring on this matches array would really make this code more understandable. For instance I couldn't really figure out what that last capture group was for until I read the tests, a variable name for it might have helped.

Bargs · 2017-04-06T22:04:22Z

src/ui/public/timepicker/parse_relative_parts.js

+      if (Math.abs(as) > 1) {
+        results.count = Math.round(Math.abs(as));
+        results.unit = units[i] + unitOp;
+        results.round = false;


Should round really always be false here? It looks like the old code set it based on a conditional:

if ($scope.from.toString().split('/')[1]) $scope.relative.round = true;

But from what I can tell, maybe that was incorrect?

Yes... Once the execution is to this point it's already NOT a relative timestamp, so we can't determine if it should be rounded or not. Default is "do not round".

Bargs · 2017-04-06T22:11:38Z

src/ui/public/timepicker/__tests__/parse_relative_parts.js

@@ -0,0 +1,105 @@
+import { parseRelativeString, parseRelativeParts } from '../parse_relative_parts';
+import { expect } from 'chai';


Have we decided to move to Chai? Afaik it's just used in Timelion. I don't wanna be a killjoy but... it gets confusing if we start mixing assertion libraries within a given plugin.

…ng to expect.js

simianhacker · 2017-04-06T23:41:43Z

@Bargs For the first thing you pointed out there is weirdness in the relative date code that has an edge case where when something is on the cusp of a whole week it returns days. If that's important we should create a separate issue and PR to fix it since it's outside the scope of this change.

I believe the rest of your concerns have been addressed.

Bargs

Had one tiny question about a variable name, otherwise LGTM!

Bargs · 2017-04-07T14:13:37Z

src/ui/public/timepicker/parse_relative_parts.js

+  let results = {};
+  const matches = _.isString(part) && part.match(/now(([\-\+])([0-9]+)([smhdwMy])(\/[smhdwMy])?)?/);
+
+  const isNotNow = matches && !matches[1];


Shouldn't this be isNow?

simianhacker · 2017-04-07T15:17:30Z

@Bargs Yeah... I'm not sure what I was thinking... fixed

* Closes elastic#2394 - Allow relative from and to * Closes elastic#6732 - Adding support future realtive for time picker

Closes elastic#2394 - Allow relative from and to

8fb74b1

simianhacker requested review from lukasolson and Bargs March 31, 2017 23:26

simianhacker assigned Bargs Mar 31, 2017

simianhacker added review v5.4.0 v6.0.0 labels Mar 31, 2017

simianhacker added 2 commits April 3, 2017 06:40

Updating tests

8b85015

restoring origin behavior for from; fixing test for to preview

a000faf

lukasolson requested changes Apr 3, 2017

View reviewed changes

Adding check to make sure to isn't before from; adding 'Set to Now'

e9e4168

lukasolson requested changes Apr 4, 2017

View reviewed changes

Adding round up to dateMath.parse for to

db89225

simianhacker added 2 commits April 4, 2017 09:58

Remove set to now on from

d733227

Removing rounding on "Set To Now" for to arguments

7cb7335

simianhacker removed the review label Apr 4, 2017

tbragin added the :Discovery label Apr 4, 2017

Closes elastic#6732 - Adding support future realtive for time picker

1b2bf60

simianhacker added the review label Apr 4, 2017

lukasolson self-assigned this Apr 5, 2017

lukasolson requested changes Apr 5, 2017

View reviewed changes

Fixing the round to label

4fbc6f6

simianhacker added 2 commits April 5, 2017 08:44

Adding error colors; adding 'Set To Now' back for from;

07cff60

Fixing tests

5d675e6

lukasolson requested changes Apr 5, 2017

View reviewed changes

simianhacker added 3 commits April 5, 2017 16:48

fixing now rounded to the nearest whatever

5ca40c4

Reformatting preview after set to now

e439479

Making the from set to now button and preview behaive the same as to

f90679e

lukasolson approved these changes Apr 6, 2017

View reviewed changes

lukasolson removed their assignment Apr 6, 2017

Preserving ranges when scrolling back in time; fixing a string bug

1c2ca32

Fixing test so it's a better reflection of what it's testing

2313f75

Bargs suggested changes Apr 6, 2017

View reviewed changes

destructuring matches for readability; smarter error checking; changi…

49d43ff

…ng to expect.js

Bargs approved these changes Apr 7, 2017

View reviewed changes

Fixing confussing name

be6dafa

simianhacker merged commit 22f8176 into elastic:master Apr 7, 2017

simianhacker added a commit to simianhacker/kibana that referenced this pull request Apr 7, 2017

Closes elastic#2394 - Allow relative from and to (elastic#10990)

c85f46d

* Closes elastic#2394 - Allow relative from and to * Closes elastic#6732 - Adding support future realtive for time picker

tbragin mentioned this pull request May 4, 2017

Relative timepicker: make "to" date customizable #11607

Closed

simianhacker deleted the issue-2394 branch April 17, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #2394 - Allow relative from and to #10990

Closes #2394 - Allow relative from and to #10990

simianhacker commented Mar 31, 2017 •

edited by lukasolson

Loading

simianhacker commented Mar 31, 2017

lukasolson left a comment

simianhacker commented Apr 3, 2017

simianhacker commented Apr 3, 2017

lukasolson left a comment

lukasolson Apr 4, 2017

simianhacker Apr 4, 2017

simianhacker commented Apr 4, 2017

simianhacker commented Apr 4, 2017

lukasolson commented Apr 4, 2017

lukasolson commented Apr 5, 2017

cjcenizal commented Apr 5, 2017

lukasolson left a comment

simianhacker commented Apr 5, 2017

simianhacker commented Apr 5, 2017

lukasolson left a comment

lukasolson left a comment

alexfrancoeur commented Apr 6, 2017

stacey-gammon commented Apr 6, 2017 •

edited

Loading

stacey-gammon commented Apr 6, 2017

simianhacker commented Apr 6, 2017

alexfrancoeur commented Apr 6, 2017

stacey-gammon commented Apr 6, 2017

bohyun-e commented Apr 6, 2017

Bargs left a comment

Bargs Apr 6, 2017

Bargs Apr 6, 2017

simianhacker Apr 6, 2017

Bargs Apr 6, 2017

simianhacker commented Apr 6, 2017

Bargs left a comment

Bargs Apr 7, 2017

simianhacker commented Apr 7, 2017

		@@ -0,0 +1,105 @@
		import { parseRelativeString, parseRelativeParts } from '../parse_relative_parts';
		import { expect } from 'chai';

Closes #2394 - Allow relative from and to #10990

Closes #2394 - Allow relative from and to #10990

Conversation

simianhacker commented Mar 31, 2017 • edited by lukasolson Loading

simianhacker commented Mar 31, 2017

lukasolson left a comment

Choose a reason for hiding this comment

simianhacker commented Apr 3, 2017

simianhacker commented Apr 3, 2017

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson Apr 4, 2017

Choose a reason for hiding this comment

simianhacker Apr 4, 2017

Choose a reason for hiding this comment

simianhacker commented Apr 4, 2017

simianhacker commented Apr 4, 2017

lukasolson commented Apr 4, 2017

lukasolson commented Apr 5, 2017

cjcenizal commented Apr 5, 2017

lukasolson left a comment

Choose a reason for hiding this comment

simianhacker commented Apr 5, 2017

simianhacker commented Apr 5, 2017

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

alexfrancoeur commented Apr 6, 2017

stacey-gammon commented Apr 6, 2017 • edited Loading

stacey-gammon commented Apr 6, 2017

simianhacker commented Apr 6, 2017

alexfrancoeur commented Apr 6, 2017

stacey-gammon commented Apr 6, 2017

bohyun-e commented Apr 6, 2017

Bargs left a comment

Choose a reason for hiding this comment

Bargs Apr 6, 2017

Choose a reason for hiding this comment

Bargs Apr 6, 2017

Choose a reason for hiding this comment

simianhacker Apr 6, 2017

Choose a reason for hiding this comment

Bargs Apr 6, 2017

Choose a reason for hiding this comment

simianhacker commented Apr 6, 2017

Bargs left a comment

Choose a reason for hiding this comment

Bargs Apr 7, 2017

Choose a reason for hiding this comment

simianhacker commented Apr 7, 2017

simianhacker commented Mar 31, 2017 •

edited by lukasolson

Loading

stacey-gammon commented Apr 6, 2017 •

edited

Loading