Close timepicker when a filter/interval is selected #9618

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

Replaces #9218.
Closes #9188.
Closes #5682.

This PR changes the behavior of the timepicker such that when you select a time filter or auto-refresh interval, the timepicker is automatically closed.

cjcenizal

Just a couple suggestions from looking at the code. I haven't tested the functionality yet.

cjcenizal · 2016-12-22T20:30:35Z

src/ui/public/timepicker/kbn_global_timepicker.js

    template: toggleHtml,
    replace: true,
-    link: ($scope) => {
+    require: '^kbnTopNav',


Do we need to require this? Can we add onFilterSelect and onIntervalSelect callbacks, and follow the same pattern as timerpicker.js? This would be a little more explicit, and less brittle.

If you took at the corresponding HTML template, you'll see kbnTopNav written all over it. I agree that the way these all tie in together is brittle, but I think it's outside of the scope of this PR to disentangle this. At very least, this require call makes it more explicit that the directive relies on being nested inside the kbnTopNav directive.

cjcenizal · 2016-12-22T20:30:47Z

src/ui/public/timepicker/timepicker.js

      interval: '=',
-      activeTab: '='
+      activeTab: '=',
+      onFilterSelect: '=',


The "Angular way" of doing this would be to refer to the callbacks as onFilterSelect: '&' and then in the markup pass in the callback like this: on-filter-select="updateFilter()".

@cjcenizal I thought about this too, but then I couldn't actually think of any real disadvantages of the way Lukas is doing it here which is much simpler to understand. Do you know what gotchas there might be with the current implementation?

It would not be possible to pass arguments like from and to if we used onFilterSelect: '&'. This looks good to React-trained eyes. 😉

@weltenwort It doesn't look like the from and to arguments are being passed anyway? But to pass them, Angular wants you to do in this super intuitive way (sarcasm :)

on-filter=select="updateFilter(from, to)"

onFilterSelect({ from, to });

@Bargs I couldn't think of any disadvantages either because it was always just "the way to do things". So I did some digging and the only thing I could find are some vague references on StackOverflow about how the expression interpreted by & is executed in the parent scope, but when interpreted by = it's executed in the call site's scope. The actual Angular docs don't really make this clear.

So I make this pen as a test. Two directives, each one representing each approach. They each receive a callback and call it immediately, which logs out this. If you open up console you can see that this evaluates to the object in which the callback is defined when using & and the directive that calls the callback when using =.

So I think the only real advantage is that & will bind the callback to the right scope.

Ah interesting, thanks for the details. All the more reason to avoid this!

cjcenizal · 2016-12-22T20:34:11Z

~~You might need to rebase master to fix the failing CI.~~ Actually, looks like we just need to update the functional tests.

Bargs · 2016-12-27T18:23:57Z

When I looked at this before I didn't realize quite how twisted up the scopes are. The fact that kbn_global_timepicker is adding functions to kbn_top_nav's scope which are then passed to the kbn-timepicker directive via the kbn_top_nav_controller is super confusing to me (let me know if I got any of that wrong, I'm still not even 100% confident).

What if we just updated kbn-timepicker to emit an event when an interval/time range is selected, have kbn_top_nav_controller listen for that event and close itself, and leave everything else alone? No modifications to kbn_global_timepicker would be required.

cjcenizal · 2016-12-27T19:08:26Z

@Bargs apologies in advance if my reply is borderline-nonsensical -- I'm technically on holiday right now so I can't dig into the code and give a well-informed suggestion. :)

I think using an event here would side-step the complexity of the way all of these top nav directives are interacting, but it seems like a band-aid, since it doesn't actually reduce the complexity. On a tactical level, I think passing callbacks to directives is more explicit and clearer than using events, so I would reach for that by default. And then from there, maybe we could try to disentangle the top nav? This is somewhat touched upon in #9050 (comment).

Again, sorry if this is not actually a helpful comment!

Bargs · 2016-12-27T20:04:42Z

Honestly I don't feel that strongly about it being an event or a callback. The point that I should have made more clearly is that I think we should avoid changing the way from/to and interval get set in this PR, because that change is forcing us to define the callbacks in kbn_global_timepicker. That makes it difficult to figure out where these callbacks are coming from, because it's kbn_top_nav_controller's scope that the filter.html and interval.html templates inherit.

Instead of an event we could just pass kbn_top_nav_controller's close method as the callback.

Bargs

@lukasolson and I chatted over zoom about my previous comment. He pointed out that there was an issue with a race condition when trying to rely on the two way binding to update the from/to properties on timefilter. If I understand correctly, the auto close would destroy the timepicker directive before the two-way binding would have a chance to update the property on the parent scope.

So with that said, I'm good with the current implementation. I left one minor inline comment. Otherwise this looks good.

Bargs · 2016-12-28T22:16:17Z

src/ui/public/timepicker/timepicker.js

      $scope.applyAbsolute = function () {
-        $scope.from = moment($scope.absolute.from);
-        $scope.to = moment($scope.absolute.to);
+        $scope.onFilterSelect($scope.absolute.from, $scope.absolute.to);


It looks like the old code was cloning the absolute dates before setting them on from/to. I wonder if we should continue to clone them before passing to onFilterSelect? I checked the git logs but there's no hint why cloning my have been necessary. The current implementation seems to work fine, but I'd worry about introducing some subtle bug if we don't make this equivalent to the old code.

cjcenizal · 2016-12-29T18:37:20Z

I'm on vacation until Monday, so I won't be able to review this again until then. If I'm blocking the merge, feel free to move forward without me. 😄

weltenwort

Since I agree that untangling the kbnTopNav would inflate this PR too much, this LGTM.

cjcenizal

Functionality and code LGTM!

Bargs · 2017-01-03T17:10:38Z

Changes look good but there appear to be a couple test failures

…on-select

tbragin · 2017-01-10T20:48:20Z

@lukasolson Nice! Let's backport to 5.3?

* Close timepicker when a filter/interval is selected * Copy absolute variables before sending * Use & instead of = for directive binding * Fix timepicker tests * Fix timepicker tests and remove tests that no longer apply

lukasolson · 2017-01-10T21:58:54Z

Backported to 5.x in 1baeae8.

Close timepicker when a filter/interval is selected

f23d6ca

lukasolson added :Discovery Feature:Timepicker Timepicker review v6.0.0 labels Dec 22, 2016

lukasolson self-assigned this Dec 22, 2016

lukasolson requested review from Bargs and weltenwort December 22, 2016 20:21

lukasolson mentioned this pull request Dec 22, 2016

Automatically close timepicker upon selection #9218

Closed

cjcenizal reviewed Dec 22, 2016

View reviewed changes

Bargs self-assigned this Dec 23, 2016

epixa added needs updates and removed review labels Dec 26, 2016

Bargs suggested changes Dec 28, 2016

View reviewed changes

lukasolson added 2 commits December 29, 2016 14:14

Copy absolute variables before sending

f7536d7

Use & instead of = for directive binding

a9bf1d1

weltenwort approved these changes Jan 2, 2017

View reviewed changes

cjcenizal approved these changes Jan 2, 2017

View reviewed changes

Bargs approved these changes Jan 3, 2017

View reviewed changes

lukasolson added 2 commits January 4, 2017 14:57

Merge branch 'master' into close-timepicker-on-select

0593681

Merge branch 'master' into close-timepicker-on-select

384776c

tbragin mentioned this pull request Jan 9, 2017

Time picker no longer closes after you select a new time range #5682

Closed

lukasolson added 2 commits January 9, 2017 15:36

Fix timepicker tests

114b598

Fix timepicker tests and remove tests that no longer apply

da1fb31

Merge remote-tracking branch 'upstream/master' into close-timepicker-…

5fa8b57

…on-select

lukasolson merged commit 3f6fe6a into elastic:master Jan 10, 2017

lukasolson added v5.3.0 and removed needs updates labels Jan 10, 2017

lukasolson mentioned this pull request Jan 17, 2017

Hide time navigation buttons #9909

Merged

lukasolson deleted the close-timepicker-on-select branch March 27, 2018 21:10

snyk-bot mentioned this pull request Apr 28, 2020

[Snyk] Security upgrade mapbox-gl from 0.54.0 to 1.10.0 larrycameron80/kibana#26

Open

snyk-bot mentioned this pull request Mar 22, 2022

[Snyk] Fix for 1 vulnerabilities larrycameron80/kibana#121

Open

larrycameron80 mentioned this pull request Nov 28, 2023

[Snyk] Fix for 3 vulnerabilities larrycameron80/kibana#177

Open

Close timepicker when a filter/interval is selected #9618

Close timepicker when a filter/interval is selected #9618

Uh oh!

Conversation

lukasolson commented Dec 22, 2016

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Bargs commented Dec 27, 2016

Uh oh!

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

Uh oh!

Uh oh!

Bargs commented Dec 27, 2016

Uh oh!

Bargs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Dec 29, 2016

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Bargs commented Jan 3, 2017

Uh oh!

tbragin commented Jan 10, 2017

Uh oh!

lukasolson commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cjcenizal commented Dec 22, 2016 •

edited

Loading

cjcenizal commented Dec 27, 2016 •

edited

Loading