Automatically close timepicker upon selection #9218

lukasolson · 2016-11-24T00:48:20Z

This PR automatically closes the timepicker when you select a time window.

The first stab at this just added a call to kbnTopNav.toggle('filter') at the end of the setQuick method, but this was problematic because the timepicker would disappear before the $scope variable changes propagated to the actual time filter. Wrapping the toggle call in a $timeout seemed to solve the problem, but I didn't like this solution.

Instead, I decided to change the way we reference the from and to values in the timepicker. Instead of having a local from and to in the timepicker's $scope, I decided to directly reference the time filter's time (which has its own from and to) and directly modify them. This way, I don't have to wrap the call to toggle in a $timeout and the changes still propagate.

Closes #9188.

tbragin · 2016-12-07T07:23:52Z

Checked it out and it looks good. Amazing how such a simple change can improve the experience.

After looking at the timepicker closer, I realized we have "Go" buttons in the Relative and Absolute selectors. What do you think about auto-closing when user clicks those buttons as well?

weltenwort · 2016-12-07T11:14:47Z

src/ui/public/timepicker/timepicker.js

    },
    template: html,
-    controller: function ($scope) {
+    require: '^kbnTopNav',


I would advise to avoid introducing such a dependency on the hierarchy the timepicker is placed in, because it limits reuse and couples the components more tightly. Why not add a hide: '&' attribute that can be used by the parent component to pass a function as in <kbn-timepicker ... hide="kbnTopNav.close('filter')">. This way the <kbn-timepicker> directive can be used in contexts where there is no kbnTopNav.

Even better would be to move from implicitly modifying the values to an explicit callback (e.g. change(newFrom, newTo) with change having been passed in by the parent) and let the parent control the visibility, but that might be out of scope for this PR.

Bargs

Agree with Tanya that we could probably auto close when the user clicks "Go" as well. Also agree with Felix that a callback would be nice. Otherwise things look good to me! (oh forgot, it does look like some tests might need to be updated though)

Bargs · 2016-12-07T21:18:22Z

src/ui/public/chrome/config/filter.html

-  from="timefilter.time.from"
-  to="timefilter.time.to"
+  time="timefilter.time"
  mode="timefilter.time.mode"


Any reason you removed from and to but not mode?

…lute

lukasolson · 2016-12-09T23:30:42Z

I've updated this PR with the suggested updates:

The time picker no longer requires the kbnTopNav in a parent scope (thanks, @weltenwort).
Instead of being passed in timefilter.time and timefilter.time.mode, the mode now is read from timefilter.time instead (thanks, @Bargs).
The time picker is now automatically closed when you apply a relative or absolute time range as well (thanks, @tbragin).

Could you guys take another look?

tbragin · 2016-12-11T01:21:41Z

For some reason, when I checked out the latest PR against master, I got server log [01:19:11.910] [error][status][plugin:[email protected]] Status changed from yellow to red - This version of Kibana requires Elasticsearch v5.2.0 on all nodes. I found the following incompatible nodes in your cluster: v6.0.0-alpha1 @ 127.0.0.1:9200 (127.0.0.1) on startup.

weltenwort · 2016-12-12T10:15:49Z

@tbragin, sounds like the version in the package.json of your working copy has been set to 5.2.0 - maybe contamination from some previous checkout?

weltenwort · 2016-12-12T10:20:30Z

src/ui/public/chrome/config/filter.html

  active-tab="'filter'"
-  interval="timefilter.refreshInterval">
+  interval="timefilter.refreshInterval"
+  on-select="kbnTopNav.toggle('filter')">


Is there any situation in which we really want to "toggle"? I feel like the intent would be better expressed by using kbnTopNav.close('filter').

Great point. I've updated the PR.

Bargs · 2016-12-12T21:50:19Z

Aside from the CI test failures, this is looking good to me.

lukasolson · 2016-12-13T15:52:55Z

As I was playing with this I realized, when users have the "Auto-refresh" tab selected and select a refresh interval, it would probably make sense to close the time picker, right?

@Bargs @weltenwort @tbragin

lukasolson · 2016-12-13T17:35:13Z

@Bargs I've finally got those pesky tests fixed. Could you take one more look?

Bargs · 2016-12-13T19:47:50Z

@lukasolson @spalger it looks like there's a linter error after the master merge

lukasolson · 2016-12-13T23:59:57Z

@Bargs Fixed.

tbragin · 2016-12-14T04:43:50Z

@lukasolson good catch on auto-refresh. yes, i think it makes sense to close it after a selection is made in that screen as well.

However, I checked out the latest PR, and when I try to set auto-refresh, the timepicker closes automatically, but auto-refresh does not seem to get set?

Bargs

Took another quick look. Looks pretty good, I think the only outstanding issue is the one Tanya mentioned about the auto refresh interval not getting applied.

tbragin · 2016-12-15T06:34:46Z

@cjcenizal I copied you on the original issue, but didn't see a reaction there. hopefully all is good with you when it comes to this change?

also cc @uboness and @kimchy - thought you might like this change :)

cjcenizal · 2016-12-15T16:58:58Z

How sure are we that this solves a problem for users without introducing a new problem? Is it possible users would want to keep the time picker open, e.g. to select different time ranges and observe the change in data? (I hate to be a stickler because I personally like this change, but I have to ask. :)

tbragin · 2016-12-15T20:40:45Z

Hey @cjcenizal - That's a fair question to ask, and that's exactly why I CC'd you!

We did discuss user's intentions when we considered this change. In majority of cases, we believe when a user makes a selection in time picker and selects a time range they intended, they would like for the time picker to close at that point.

However there are situations, where it's advantageous to keep the time picker open, so let's talk about some of them.

The first situation I can think of is when the user wants to keep changing the time. An example of that might be navigating forward and backward in time. Luckily, for that purpose we just introduced previous / next time navigation buttons in time picker that are a better solution to that problem than manually changing the time frame repeatedly.
The other main use case i can imagine is when a user makes a selection that is too wide or too narrow. In the case of too wide, I think the user can simply brush one of date histogram based charts on the screen to the time range they want, assuming that is present (it is in Discover and in most dashboards based on time-based data). Zooming out is harder (at least until/if we introduce zoom out controls), but for now, say if you selected "last 2 years" and see no data, and realize that your data is actually 3 years back -- you can still use the previous button to get to where you need to be with one click.

In summary, we do think that in majority of use cases, this change will be beneficial, and the saved clicks in cases where user made the proper selection the first time will outweigh the few added clicks when the user made a mistake.

Does that make sense?

cjcenizal · 2016-12-15T21:04:49Z

@tbragin Thanks for the analysis! Sounds to me like this is a good change to make.

cjcenizal · 2016-12-19T17:44:06Z

Does this address #5682 ?

lukasolson · 2016-12-20T21:04:44Z

@cjcenizal I believe so, although I don't remember that ever being the expected behavior before this PR... Strange.

lukasolson · 2016-12-22T20:21:37Z

Replaced by #9618 (less invasive solution).

- `@elastic/eui`: `v109.1.0` ⏩ `v109.2.0` - `@elastic/eui-theme-borealis`: `v5.0.0` ⏩ `v5.1.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) --- ## Changes - Only snapshot updates related to EuiPopover, ToolTip and Table changes (see below) ## Package updates ### `@elastic/eui` v109.2.0 - Updated `EuiFlexItem` to fall back to `grow={true}` if invalid values for `grow` are passed ([#9228](elastic/eui#9228)) - Updated shared button styles in `useEuiButtonColorCSS` to use `euiDisabledSelector` ([#9226](elastic/eui#9226)) - Added `euiTextTruncateCSS` Emotion style utility ([#9231](elastic/eui#9231)) - Added `hasBackground` prop on `EuiTable`, `EuiBasicTable` and `EuiInMemoryTable` ([#9224](elastic/eui#9224)) - Added component token `components.tableFooterBackground` ([#9224](elastic/eui#9224)) - Updated the color of mobile table header cells to use `colors.textSubdued` ([#9224](elastic/eui#9224)) - Updated `EuiSuperDatePicker` to show a tooltip with the full range details when the button displays a pretty duration e.g. "Last 15 minutes" ([#9221](elastic/eui#9221)) - Updated `EuiPopover` default prop values of `hasArrow`, `position` and `offset`: ([#9218](elastic/eui#9218)) - Changed `hasArrow` to `false` - Changed `position` to `downLeft` - Changed `offset` to `4` when `hasArrow=false` - Updated `EuiInputPopover` `offset` default value to `2` ([#9218](elastic/eui#9218)) - Updated `EuiTourStep` to not apply `hasArrow=true` by default when `decoration="none"` ([#9218](elastic/eui#9218)) - Updated `EuiSuperDatePicker` to have a more forgiving manual input for absolute dates. ([#9199](elastic/eui#9199)) **Bug fixes** - Updated EuiButtonGroup disabled style selectors to use `euiDisabledSelector` to ensure high contrast mode styles apply correctly ([#9226](elastic/eui#9226)) - Updated `EuiSuperDatePicker` to ensure its pretty format button dates are truncated correctly ([#9231](elastic/eui#9231)) - Fixed a visual bug for mobile table action buttons that causes shifting positions when changing color mode ([#8231](elastic/eui#8231)) ([#9224](elastic/eui#9224)) **Accessibility** - Improved the navigation of sibling `EuiToolTip` anchor elements in NVDA browse mode by adding an `id` to ensure they are unique ([#9208](elastic/eui#9208)) ### `@elastic/eui-theme-borealis` v5.1.0 - Added component token `components.tableFooterBackground` ([#9224](elastic/eui#9224)) --------- Co-authored-by: Jorge Sanz <[email protected]> Co-authored-by: Lene Gadewoll <[email protected]>

- `@elastic/eui`: `v109.1.0` ⏩ `v109.2.0` - `@elastic/eui-theme-borealis`: `v5.0.0` ⏩ `v5.1.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) --- ## Changes - Only snapshot updates related to EuiPopover, ToolTip and Table changes (see below) ## Package updates ### `@elastic/eui` v109.2.0 - Updated `EuiFlexItem` to fall back to `grow={true}` if invalid values for `grow` are passed ([elastic#9228](elastic/eui#9228)) - Updated shared button styles in `useEuiButtonColorCSS` to use `euiDisabledSelector` ([elastic#9226](elastic/eui#9226)) - Added `euiTextTruncateCSS` Emotion style utility ([elastic#9231](elastic/eui#9231)) - Added `hasBackground` prop on `EuiTable`, `EuiBasicTable` and `EuiInMemoryTable` ([elastic#9224](elastic/eui#9224)) - Added component token `components.tableFooterBackground` ([elastic#9224](elastic/eui#9224)) - Updated the color of mobile table header cells to use `colors.textSubdued` ([elastic#9224](elastic/eui#9224)) - Updated `EuiSuperDatePicker` to show a tooltip with the full range details when the button displays a pretty duration e.g. "Last 15 minutes" ([elastic#9221](elastic/eui#9221)) - Updated `EuiPopover` default prop values of `hasArrow`, `position` and `offset`: ([elastic#9218](elastic/eui#9218)) - Changed `hasArrow` to `false` - Changed `position` to `downLeft` - Changed `offset` to `4` when `hasArrow=false` - Updated `EuiInputPopover` `offset` default value to `2` ([elastic#9218](elastic/eui#9218)) - Updated `EuiTourStep` to not apply `hasArrow=true` by default when `decoration="none"` ([elastic#9218](elastic/eui#9218)) - Updated `EuiSuperDatePicker` to have a more forgiving manual input for absolute dates. ([elastic#9199](elastic/eui#9199)) **Bug fixes** - Updated EuiButtonGroup disabled style selectors to use `euiDisabledSelector` to ensure high contrast mode styles apply correctly ([elastic#9226](elastic/eui#9226)) - Updated `EuiSuperDatePicker` to ensure its pretty format button dates are truncated correctly ([elastic#9231](elastic/eui#9231)) - Fixed a visual bug for mobile table action buttons that causes shifting positions when changing color mode ([elastic#8231](elastic/eui#8231)) ([elastic#9224](elastic/eui#9224)) **Accessibility** - Improved the navigation of sibling `EuiToolTip` anchor elements in NVDA browse mode by adding an `id` to ensure they are unique ([elastic#9208](elastic/eui#9208)) ### `@elastic/eui-theme-borealis` v5.1.0 - Added component token `components.tableFooterBackground` ([elastic#9224](elastic/eui#9224)) --------- Co-authored-by: Jorge Sanz <[email protected]> Co-authored-by: Lene Gadewoll <[email protected]>

Automatically close timepicker upon quick selection

ebd119b

lukasolson added WIP Work in progress tests_needed v5.2.0 v6.0.0 labels Nov 24, 2016

lukasolson self-assigned this Nov 24, 2016

lukasolson added 3 commits December 6, 2016 15:37

Merge branch 'master' into timepicker/auto-close

90d84be

Use direct reference to timepicker time instead of separate from/to

f5b5bcd

remove unused variable

d536ee1

lukasolson added review and removed WIP Work in progress tests_needed labels Dec 7, 2016

lukasolson assigned weltenwort and Bargs and unassigned lukasolson Dec 7, 2016

weltenwort reviewed Dec 7, 2016

View reviewed changes

Bargs reviewed Dec 7, 2016

View reviewed changes

lukasolson assigned lukasolson and unassigned weltenwort and Bargs Dec 8, 2016

lukasolson added 3 commits December 8, 2016 16:50

Merge branch 'master' into timepicker/auto-close

8da883e

Merge branch 'master' into timepicker/auto-close

6797474

Remove dependency on kbnTopNav, use time.mode, close on relative/abso…

bff207d

…lute

lukasolson changed the title ~~Automatically close timepicker upon quick selection~~ Automatically close timepicker upon selection Dec 9, 2016

weltenwort reviewed Dec 12, 2016

View reviewed changes

lukasolson added 2 commits December 13, 2016 08:53

Close time picker when selecting a refresh interval

31f5ca9

Don't click time picker a second time in tests to hide it

230bd98

Merge branch 'master' into timepicker/auto-close

e49918b

Merge branch 'master' into timepicker/auto-close

48d8855

tbragin added the :Discovery label Dec 14, 2016

Bargs suggested changes Dec 14, 2016

View reviewed changes

Merge branch 'master' into timepicker/auto-close

05cd0be

lukasolson closed this Dec 22, 2016

lukasolson deleted the timepicker/auto-close branch December 22, 2016 20:18

lukasolson restored the timepicker/auto-close branch December 22, 2016 20:18

lukasolson reopened this Dec 22, 2016

lukasolson mentioned this pull request Dec 22, 2016

Close timepicker when a filter/interval is selected #9618

Merged

lukasolson closed this Dec 22, 2016

lukasolson mentioned this pull request Jan 17, 2017

Hide time navigation buttons #9909

Merged

lukasolson deleted the timepicker/auto-close branch March 27, 2018 21:10

Automatically close timepicker upon selection #9218

Automatically close timepicker upon selection #9218

Uh oh!

Conversation

lukasolson commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbragin commented Dec 7, 2016

Uh oh!

weltenwort Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

Bargs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bargs Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

lukasolson commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbragin commented Dec 11, 2016

Uh oh!

weltenwort commented Dec 12, 2016

Uh oh!

weltenwort Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

lukasolson Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

Bargs commented Dec 12, 2016

Uh oh!

lukasolson commented Dec 13, 2016

Uh oh!

lukasolson commented Dec 13, 2016

Uh oh!

Bargs commented Dec 13, 2016

Uh oh!

lukasolson commented Dec 13, 2016

Uh oh!

tbragin commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bargs left a comment

Choose a reason for hiding this comment

Uh oh!

tbragin commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjcenizal commented Dec 15, 2016

Uh oh!

tbragin commented Dec 15, 2016

Uh oh!

cjcenizal commented Dec 15, 2016

Uh oh!

cjcenizal commented Dec 19, 2016

Uh oh!

lukasolson commented Dec 20, 2016

Uh oh!

lukasolson commented Dec 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lukasolson commented Nov 24, 2016 •

edited

Loading

Bargs left a comment •

edited

Loading

lukasolson commented Dec 9, 2016 •

edited

Loading

tbragin commented Dec 14, 2016 •

edited

Loading

tbragin commented Dec 15, 2016 •

edited

Loading