Timepicker - add 'recently selected' range option #15868

nreese · 2018-01-05T21:46:26Z

Store time history in local storage. Cached values selectable under Recent tab.

lukasolson

Let me first just say thanks for taking this on! This is awesome.

I think it would be worthwhile to hop on a video chat with @snide and @Bargs and chat about these changes. I'm concerned with a couple things:

Now there are two different places where default time intervals are stored (local storage and the advanced setting). Which should take precedence?
I think the UI for selecting a previous time interval is a tad confusing, because a user might expect that hitting "Go" is no longer how to apply the interval you've selected (since now there's a drop-down next to the button that says "Select a previously selected interval".
I'm not sure we need to use three different local storage keys to accomplish this... what if we just had a log, and if we wanted to default to the most recent, just grab the most recent from the log?

I'd love to have a quick 30-60 min meeting and discuss these things so we can all be on the same page.

lukasolson · 2018-01-08T18:44:47Z

src/ui/public/timefilter/time_history.js

+
+class TimeHistory {
+  constructor() {
+    this.storage = new Storage(window.localStorage);


Instead of using the Storage class, it might make more sense to use the PersistedLog class, which already takes care of duplicates.

Bargs · 2018-01-09T22:37:59Z

I'll add a couple thoughts here too so I don't forget during the zoom:

I think history would probably be useful in the relative mode as well, so I'm wondering if it should just be a log of all recent time ranges, regardless of what mode it came from.
We've talked with Dave in the past about moving the timepicker into the query bar, so everything affecting the query is in one place. Once we do that, I'm not sure it would make sense for the timepicker to have it's own independent history. Its history would probably just get tracked in the query bar's history. So that's something to consider before implementing a separate history mechanism here.

nreese · 2018-01-12T03:14:47Z

@lukasolson @Bargs @snide @cjcenizal As discussed, I have updated the PR to insert the time history into the quick timepicker select. What do you think? Is this a better UI?

cjcenizal · 2018-01-12T17:54:39Z

My initial reaction is that this becomes cramped and difficult to read. The ranges themselves are so long that they wrap, so its hard to tell one range from another. Also, it's hard to tell at a glance where the static "quick" options end and the dynamic "cached" ranges begin. I think we should add a fourth tab called "Recent ranges" and move this content there.

nreese · 2018-01-13T01:17:00Z

@cjcenizal I have moved the time history to its own tab in the time picker. Does this help solve some of the spacing issues?

cjcenizal

Nice work! I had a few suggestions for you, all optional. I also noticed that when there are no recent ranges, it looks a bit broken:

How about adding an empty state? Just some text saying, "When you specify new time changes they'll be saved here." CC @gchaps for better suggestions on this copy.

cjcenizal · 2018-01-16T02:19:49Z

src/ui/public/persisted_log/persisted_log.js

+const localStorage = new Storage(window.localStorage);
+
+export class PersistedLog {
+  constructor(name, options, storage) {


We can specify defaults like this to be more efficient:

constructor(name, options = {}, storage = localStorage) {

A bit outside the scope of the PR but not that big a change.

cjcenizal · 2018-01-16T02:22:31Z

src/ui/public/timefilter/time_history.js

+import { PersistedLog } from 'ui/persisted_log';
+import { TIME_MODES } from 'ui/timepicker/modes';
+
+class TimeHistory {


Want to add tests for this? They could be written with Jest.

cjcenizal · 2018-01-16T02:23:34Z

src/ui/public/persisted_log/__tests__/persisted_log.js

-    PersistedLog = $injector.get('PersistedLog');
-  });
-}
-
 describe('PersistedLog', function () {


Perhaps outside the scope of this PR, but this test can be converted to Jest by renaming it persisted_log.test.js and moving it out of this directory into the same directory as persisted_log.js. And then all the it calls can be renamed to be test, which is purely cosmetic.

nreese · 2018-01-16T16:39:07Z

@cjcenizal Added a message for when time history is empty

nreese · 2018-01-16T16:46:27Z

@lukasolson @Bargs Other then some tests, this PR is ready for review

gchaps · 2018-01-16T18:32:50Z

@nreese @cjcenizal

How about making the text a little more concise? I'm wondering if the wording about absolute/relative and 10 views might be too much detail for an empty state. They are typically short.

When you specify a relative or absolute time range, it shows up here.

or

Time ranges show up here, so you can easily view them later

Other options to consider

To add a time range, click the New button above

—
When you have a time range, you’ll see it here

—
No time history

Your most recently used time ranges show up here so you can view them later
—

nreese · 2018-01-16T19:04:37Z

@gchaps Thanks for suggestions. I used the last option provided.

cjcenizal · 2018-01-16T21:02:50Z

@gchaps I love the wording. The hierarchy of the text looks a little odd to me since there's no title treatment on the first sentence. But do we need a title? Can this just be two sentence on one line? "No time history. Your most recently used time ranges show up here so you can view them later."

gchaps · 2018-01-16T22:52:15Z

@cjcenizal Yes, one line works.

@nreese Can I tweak it a bit? I missed a comma. I also tweaked it a bit to make the two lines read better together.

No time history. Your most recent time ranges show up here, so you can easily view them later.

nreese · 2018-01-16T22:56:00Z

@gchaps @cjcenizal Thanks

lukasolson

Could we update the text to always mirror what shows in the timepicker? For example, I've selected a relative range of now-20m to now. The timepicker says "Last 20m". Can we show that text instead of "now-20m to now"? I think it makes more sense, and that seems to be the behavior for the absolute ranges.

Also, I'd rather it just say "Recent" or "History" and it would make more sense to me to add it to the far right or far left, but I could live with it as-is. Thoughts, @cjcenizal?

lukasolson · 2018-01-16T23:40:20Z

src/ui/public/timepicker/timepicker.js

@@ -35,10 +37,10 @@ module.directive('kbnTimepicker', function (timeUnits, refreshIntervals) {
    template: html,
    controller: function ($scope) {
      $scope.format = 'MMMM Do YYYY, HH:mm:ss.SSS';
-      $scope.modes = ['quick', 'relative', 'absolute'];
+      $scope.modes = [TIME_MODES.QUICK, TIME_MODES.RECENT, TIME_MODES.RELATIVE, TIME_MODES.ABSOLUTE];


I'd prefer the following:

$scope.modes = Object.values(TIME_MODES);

Then, if someone adds a new mode for some reason, this line doesn't have to change, and it's more concise.

Good idea, we're using a similar pattern in EUI.

cjcenizal · 2018-01-17T00:42:30Z

@lukasolson Sounds good to me. How about "Recent", which will reinforce the reference to "recent ranges"? "History" sounds a bit ambiguous.

gchaps · 2018-01-17T01:50:23Z

@lukasolson I agree with @cjcenizal to use "Recent" . "History" sounds like it will show everything where I think this will only show the top 10.

nreese · 2018-01-17T19:48:04Z

@lukasolson Great suggestion to mirror the timepicker time range display. Now the recent panel and the timepicker share the same logic for generating the display strings. This makes the recent panel look much nicer.

Bargs · 2018-01-17T19:56:33Z

Any idea how we could visually distinguish each entry? With long absolute ranges wrapping I find it a little difficult to see where one entry ends and the next begins.

Bargs · 2018-01-17T19:58:18Z

Maybe an easy solution would be to display the recently used dates in a single column instead of a two column layout, that way the absolute dates wouldn't have to wrap? Just a thought I had immediately after posting the previous comment, maybe there are better solutions.

cjcenizal · 2018-01-17T19:59:26Z

I think we need to adjust the line-height so that the vertical space between multiple lines in a single entry is smaller than the space between entries. Let me try something out locally.

cjcenizal · 2018-01-18T16:47:33Z

@Bargs Are you referring to the column width and the odd line-wrapping? I think the only that stands out to me as solvable in that screenshot is the column width. The column widths should definitely be the same. In terms of odd line-wrapping (e.g. orphans being left on the last line), I think that's unavoidable.

Bargs · 2018-01-18T17:44:05Z

I'm mainly talking about the odd line wrapping. It's only unavoidable if we stick to a two column layout. What is the advantage of having two columns? It's not really saving much vertical space since most lines will wrap with two columns. And if we cap the history at some limit (say, the last 10 entries), it's never going to take too much vertical space.

cjcenizal · 2018-01-18T17:56:18Z

@Bargs Sounds good. I don't feel strongly about this. I'll let you and Nathan work it out.

nreese · 2018-01-18T18:57:07Z

@Bargs I have updated the recent panel to be displayed in a single column. This should avoid all wrapping concerns.

Bargs

I left one suggestion for the feature itself, but all the code LGTM!

Bargs · 2018-01-18T22:40:35Z

src/ui/public/timefilter/time_history.js

+  }
+
+  add(time) {
+    if (!time || time.mode === TIME_MODES.QUICK) {


I'd argue it makes sense to include quick times in the history for the sake of consistency. When I first pulled down the PR and started playing with it I started switching between Quick options and I thought something was broken because they weren't showing up in the Recent list. I think a user might want to be able to flip back and forth between a couple quick options without having to find it in the giant Quick list each time. Might just be me though, if others disagree I won't be insistent.

That makes sense. I will remove this check

lukasolson

LGTM! Thanks!

nreese · 2018-01-22T15:20:48Z

jenkins, test this

…can be shared by recent history view

* store time filter history in localStorage * allow selection of relative history * use persisted log for storage * display time history in quick list * add recent timepicker panel * set timehistory mode to recent * add message to display when no time history exists yet * migrate persisted_logs tests from mocha to jest * reword text for no history * better wording * move logic from pretty-duration directive into its own file so logic can be shared by recent history view * some small changes * fix list styling * make recent be a single column * store quick times in history * set default timezone for moment in pretty_duration tests

elasticmachine · 2018-01-29T17:01:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nreese added Feature:Timepicker Timepicker release_note:enhancement labels Jan 5, 2018

nreese requested review from lukasolson and Bargs January 5, 2018 21:59

lukasolson requested changes Jan 8, 2018

View reviewed changes

nreese force-pushed the timefilter_cache branch from 8e91a21 to d45ebad Compare January 10, 2018 18:31

lukasolson mentioned this pull request Jan 10, 2018

Add advanced setting to control quick ranges #15975

Merged

nreese force-pushed the timefilter_cache branch from 5891fea to 3fbdb67 Compare January 12, 2018 18:51

cjcenizal approved these changes Jan 16, 2018

View reviewed changes

nreese force-pushed the timefilter_cache branch from 7b847f8 to 35ceb58 Compare January 16, 2018 16:17

lukasolson requested changes Jan 16, 2018

View reviewed changes

nreese force-pushed the timefilter_cache branch from 99c5f54 to e7af3b7 Compare January 17, 2018 18:18

nreese force-pushed the timefilter_cache branch from 54818ab to ee409e2 Compare January 18, 2018 18:48

Bargs approved these changes Jan 18, 2018

View reviewed changes

lukasolson approved these changes Jan 18, 2018

View reviewed changes

nreese added 16 commits January 22, 2018 08:51

store time filter history in localStorage

5313583

allow selection of relative history

9005d96

use persisted log for storage

de1ceb8

display time history in quick list

c1be680

add recent timepicker panel

f6425a8

set timehistory mode to recent

9561abb

add message to display when no time history exists yet

5552328

migrate persisted_logs tests from mocha to jest

ec6dde4

reword text for no history

b286e22

better wording

c6175cd

move logic from pretty-duration directive into its own file so logic …

4068290

…can be shared by recent history view

some small changes

06b5b15

fix list styling

5cb08d1

make recent be a single column

324d1be

store quick times in history

e840163

set default timezone for moment in pretty_duration tests

908efb6

nreese force-pushed the timefilter_cache branch from 69dec42 to 908efb6 Compare January 22, 2018 15:51

nreese merged commit dc55138 into elastic:master Jan 22, 2018

nreese mentioned this pull request Jan 22, 2018

[6.x] Timepicker - add 'recently selected' range option (#15868) #16204

Merged

Timepicker - add 'recently selected' range option #15868

Timepicker - add 'recently selected' range option #15868

Conversation

nreese commented Jan 5, 2018 • edited Loading

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson Jan 8, 2018

Choose a reason for hiding this comment

Bargs commented Jan 9, 2018

nreese commented Jan 12, 2018

cjcenizal commented Jan 12, 2018

nreese commented Jan 13, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Jan 16, 2018

Choose a reason for hiding this comment

cjcenizal Jan 16, 2018

Choose a reason for hiding this comment

cjcenizal Jan 16, 2018

Choose a reason for hiding this comment

nreese commented Jan 16, 2018

nreese commented Jan 16, 2018

gchaps commented Jan 16, 2018 • edited Loading

nreese commented Jan 16, 2018

cjcenizal commented Jan 16, 2018

gchaps commented Jan 16, 2018

nreese commented Jan 16, 2018

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson Jan 16, 2018

Choose a reason for hiding this comment

cjcenizal Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Jan 17, 2018

gchaps commented Jan 17, 2018

nreese commented Jan 17, 2018

Bargs commented Jan 17, 2018

Bargs commented Jan 17, 2018

cjcenizal commented Jan 17, 2018 • edited Loading

cjcenizal commented Jan 18, 2018

Bargs commented Jan 18, 2018

cjcenizal commented Jan 18, 2018

nreese commented Jan 18, 2018 • edited Loading

Bargs left a comment

Choose a reason for hiding this comment

Bargs Jan 18, 2018

Choose a reason for hiding this comment

nreese Jan 18, 2018

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

nreese commented Jan 22, 2018

elasticmachine commented Jan 29, 2018

💚 Build Succeeded

nreese commented Jan 5, 2018 •

edited

Loading

gchaps commented Jan 16, 2018 •

edited

Loading

cjcenizal Jan 17, 2018 •

edited

Loading

cjcenizal commented Jan 17, 2018 •

edited

Loading

nreese commented Jan 18, 2018 •

edited

Loading