Implement template notifications by tsullivan · Pull Request #7781 · elastic/kibana

tsullivan · 2016-07-20T20:24:31Z

Replaces #7670
Fixes #6933.

Adds a custom function on the notifier that gives granular control to the user.
Custom Notifiers can have custom actions
- Custom actions can execture their own code, in their own scope
- Custom actions always close the notification when clicked

In order to use these notifications one must do something similar to the following code

import Notifier from 'ui/notify/notifier';
const notify = new Notifier({location: 'AppName'});
notify.custom('Some markdown content', {
  type: 'info',
  actions: [{
    text: 'More Info',
    callback: function() { window.alert('hello world'); }
  }, {
    text: 'Cancel',
    callback: function() { fooCancelBar(); }
  }]
});

All of the callbacks are wrapped in a close function. If you wish to display new information as a result of the action, make a new notification by calling notify.type_of_notification() in the callback.

w33ble · 2016-07-26T23:03:34Z

Sent you some changes. tsullivan#1

ycombinator · 2016-07-27T16:56:39Z

Assigning myself as a reviewer.

tsullivan · 2016-07-27T17:55:12Z

src/ui/public/notify/notifier.js

I left comments on a PR that was merged into this one about these conditions. This is helping to get the blocker issue fixed by not breaking an API, but I dug around and can't really see anywhere that error info and warning notifications actually use the callback param anywhere.

Left a comment back there too, but I'll re-post it here.

I didn't see any in our code either, but it's possible plugins use it. I figured it best not to break the API until we take a bigger stab at updating Notifications.

👍 Good point about plugins, and that we're planning to re-do Notifications as a whole in the near future.

w33ble · 2016-07-27T18:10:12Z

src/ui/public/notify/notifier.js

Since the other "helper" methods take 2 parameters, this one should probably throw if config here isn't an object, so that nobody tries to pass a callback as the second parameter. For the custom notification, not passing a config object doesn't make a lot of sense...

w33ble · 2016-07-27T18:13:15Z

I updated the description in this PR to reflect the modified API of the custom method.

…ol over notifications

just run all content through the markdown parser, and add margin to alight text correctly

check value types, more tests with less concerns, clean up notifications after each test

tsullivan · 2016-07-27T23:45:18Z

I tested with type: 'error' but I get the white/missing background instead of the expected red one:

Apparently, to get the "error" type as we call it, the type passed must be danger.

I think that inconsistency was never noticed because previously type was totally internal. Now that we can pass type, I can think of 2 options to clear things:

make it so passing type: 'error' has the right effect
rename the internal danger value to just error - there may be a reason why it wasn't called error to begin with though, it could be some CSS conflict.

ycombinator · 2016-07-27T23:53:21Z

rename the internal danger value to just error - there may be a reason why it wasn't called error to begin with though, it could be some CSS conflict.

I'd at least attempt this option first. If there are no conflicts, I think this is the saner option, for consistency sake.

tsullivan · 2016-07-28T00:59:10Z

I'd at least attempt this option first. If there are no conflicts, I think this is the saner option, for consistency sake.

Looks like the keyword danger was chosen to match a very bootstrap theme which is at a very high level, so that usage is in tons of places in the HTML/CSS: https://github.com/elastic/kibana/blob/master/src/ui/public/styles/bootstrap/theme.less#L207

Trying to make this usage consistent with how we think of it as an error would mean either changing or duplicating a lot of unrelated code. It would also make the Bootstrap layer inconsistent, where we might have alert-error and alert-danger as identical, but there wouldn't be a panel-error that duplicates panel-danger.

Therefore, I think option 1 is the right way to go. Even though if you put notifier.js in isolation, option 2 looks better.

ycombinator · 2016-07-28T02:30:47Z

Thanks for looking into option 2 @tsullivan. Sounds like option 1 it is!

tsullivan · 2016-07-28T22:02:52Z

One thing I would like to do with the custom notification is to have a config option that can change the truncation length of the message. I think that is out of scope for this PR because it doesn't target the blocker this PR is meant to address. So that will be in a future PR.

tsullivan · 2016-07-28T23:11:13Z

@ycombinator @cjcenizal @w33ble ready for another review

ycombinator · 2016-07-29T00:57:00Z

Functionality (still) looks great! Reviewing code again...

ycombinator · 2016-07-29T01:02:25Z

LGTM!

cjcenizal · 2016-07-30T00:10:22Z

src/ui/public/notify/partials/toaster.html

+            type="button"
+            class="btn"
+            ng-repeat="action in notif.customActions"
+            ng-class="'btn-' + notif.type"


This may be beyond the scope of this PR, but I think it will need to be done someday: I think we should be more explicit in our class assignment, e.g. via methods like getButtonClass(notif.type) and getToastClass(notif.type). This will uncouple the type from the class names for these elements, which will let us remove the 'error'/'danger' substitution on line 431. This will also give us to change the implementation details of these different types without having to change the type values in the future.

These methods could look something like this:

// In kbnNotifications directive's link function const typeToButtonClassMap = { info: 'btn-info', warning: 'btn-warning', error: 'btn-danger', }; const typeToToastClassMap = { info: 'alert-info', warning: 'alert-warning', error: 'alert-danger', }; scope.getButtonClass(type) { return typeToButtonClassMap[type] || 'btn-info'; } scope.getToastClass(type) { return typeToToastClassMap[type] || 'alert-info'; }

I plan to have a follow-up PR that will allow custom control over the truncation length of the message. I can add a minor clean-up task there in that PR.

It could also be a standalone PR though - no reason for it not to be

Sounds good to me

cjcenizal · 2016-07-30T00:15:23Z

I only had one small suggestion. LGTM otherwise!

w33ble · 2016-08-01T22:27:10Z

Thanks for seeing this PR all the way through!

tsullivan · 2016-08-01T23:02:48Z

Sure thing, @w33ble. I forgot to squash commits before merge though. Whoops.

--------- **Commit 1:** [Notifier] Added a custom notify function which allows complete control over notifications * Original sha: 33bf38f * Authored by Khalah Jones Golden <khalah@elastic.co> on 2016-07-07T20:40:08Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 2:** custom notifications: use `text` as property for action button label * Original sha: 3c42f2d * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-20T22:10:53Z **Commit 3:** remove distinction between content and markdown just run all content through the markdown parser, and add margin to alight text correctly * Original sha: 864cf9d * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-25T21:58:11Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 4:** fix timeout starting at 0 bug * Original sha: 702c09a * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-25T22:11:04Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 5:** allow 0 to be used to disable timeout * Original sha: eeeb94e * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-25T22:12:06Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 6:** allow property overrides on helper methods * Original sha: b0584ef * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-25T22:46:22Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 7:** make custom notify api closer to other helper apis * Original sha: 932751a * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-25T22:54:56Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 8:** update custom notification tests * Original sha: 7055f6c * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-25T22:57:21Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 9:** punch up the tests check value types, more tests with less concerns, clean up notifications after each test * Original sha: 47edbfd * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-26T00:12:29Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 10:** fix issue with custom notification handling * Original sha: 507429f * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-26T00:12:46Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 11:** update the docblock * Original sha: a9a0362 * Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-07-26T00:19:57Z * Committed by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T19:08:49Z **Commit 12:** Custom notification: throw error if config object is invalid * Original sha: d138de1 * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T21:38:39Z **Commit 13:** Notifier Config: move overridableOptions to higher level * Original sha: 3ccde29 * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T22:59:29Z **Commit 14:** switch order of negative/postive condition blocks * Original sha: 7ea3eee * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T23:12:53Z **Commit 15:** remove unecessary ng-if * Original sha: 33c41e2 * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T23:21:13Z **Commit 16:** remove limit of number of actions * Original sha: 259c480 * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T23:27:11Z **Commit 17:** custom notifier dynamic lifetime based on type * Original sha: 03aa6b8 * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-27T23:49:39Z **Commit 18:** test cleanup per feedback * Original sha: dbea052 * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-28T00:20:44Z **Commit 19:** allow passing of `type: ‘error’` * Original sha: 0c7c339 * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-28T01:55:55Z **Commit 20:** add tests for latest enhancements * Original sha: 7e99d6c * Authored by Timothy Sullivan <tsullivan@elastic.co> on 2016-07-28T23:10:48Z

[backport] PR #7781 to 4.x

…ications Implement template notifications Former-commit-id: 2adb8d4

tsullivan added release_note:enhancement blocker review P1 labels Jul 20, 2016

tsullivan mentioned this pull request Jul 20, 2016

Custom Notifications #7670

Closed

tsullivan changed the title ~~Feature template notifications~~ Implement template notifications Jul 20, 2016

epixa added v5.0.0-beta1 and removed P1 release_note:enhancement labels Jul 26, 2016

ycombinator self-assigned this Jul 27, 2016

ycombinator removed the review label Jul 27, 2016

tsullivan reviewed Jul 27, 2016
View reviewed changes

tsullivan force-pushed the feature-template-notifications branch from 5976468 to f30ef66 Compare July 27, 2016 18:06

w33ble mentioned this pull request Jul 27, 2016

Feature template notifications tweaks tsullivan/kibana#1

Merged

w33ble reviewed Jul 27, 2016
View reviewed changes

w33ble assigned cjcenizal Jul 27, 2016

Khalah Jones Golden and others added 11 commits July 27, 2016 12:08

[Notifier] Added a custom notify function which allows complete contr…

33bf38f

…ol over notifications

custom notifications: use text as property for action button label

3c42f2d

remove distinction between content and markdown

864cf9d

just run all content through the markdown parser, and add margin to alight text correctly

fix timeout starting at 0 bug

702c09a

allow 0 to be used to disable timeout

eeeb94e

allow property overrides on helper methods

b0584ef

make custom notify api closer to other helper apis

932751a

update custom notification tests

7055f6c

punch up the tests

47edbfd

check value types, more tests with less concerns, clean up notifications after each test

fix issue with custom notification handling

507429f

update the docblock

a9a0362

tsullivan added 2 commits July 27, 2016 16:21

remove unecessary ng-if

33c41e2

remove limit of number of actions

259c480

custom notifier dynamic lifetime based on type

03aa6b8

test cleanup per feedback

dbea052

allow passing of type: ‘error’

0c7c339

add tests for latest enhancements

7e99d6c

ycombinator removed their assignment Jul 29, 2016

cjcenizal reviewed Jul 30, 2016
View reviewed changes

tsullivan merged commit 2adb8d4 into elastic:master Jul 30, 2016

epixa added the v4.6.0 label Aug 3, 2016

elastic-jasper mentioned this pull request Aug 3, 2016

[backport] PR #7781 to 4.x - Implement template notifications #7916

Merged

epixa added v5.0.0-alpha5 and removed v5.0.0-beta1 labels Aug 4, 2016

cjcenizal mentioned this pull request Aug 7, 2016

Ability to set the position of legends #7931

Merged

w33ble added a commit that referenced this pull request Aug 9, 2016

Merge pull request #7916 from elastic/jasper/backport/7781/4.x

e4eb59b

[backport] PR #7781 to 4.x

spalger mentioned this pull request Aug 19, 2016

[notifier] revert removal of status reporting #8026

Merged

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#7781 from tsullivan/feature-template-notif…

509333a

…ications Implement template notifications Former-commit-id: 2adb8d4

tsullivan deleted the feature-template-notifications branch July 19, 2018 05:21

Conversation

tsullivan commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w33ble commented Jul 26, 2016

Uh oh!

ycombinator commented Jul 27, 2016

Uh oh!

tsullivan Jul 27, 2016

Choose a reason for hiding this comment

Uh oh!

w33ble Jul 27, 2016 • 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!

tsullivan Jul 27, 2016

Choose a reason for hiding this comment

Uh oh!

w33ble Jul 27, 2016

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tsullivan commented Jul 27, 2016

Uh oh!

ycombinator commented Jul 27, 2016

Uh oh!

tsullivan commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented Jul 28, 2016

Uh oh!

tsullivan commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsullivan commented Jul 28, 2016

Uh oh!

ycombinator commented Jul 29, 2016

Uh oh!

ycombinator commented Jul 29, 2016

Uh oh!

cjcenizal Jul 30, 2016 • 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!

tsullivan Jul 30, 2016

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jul 30, 2016

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Jul 30, 2016

Uh oh!

w33ble commented Aug 1, 2016

Uh oh!

tsullivan commented Aug 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tsullivan commented Jul 20, 2016 •

edited

Loading

w33ble Jul 27, 2016 •

edited

Loading

w33ble commented Jul 27, 2016 •

edited

Loading

tsullivan commented Jul 28, 2016 •

edited

Loading

tsullivan commented Jul 28, 2016 •

edited

Loading

cjcenizal Jul 30, 2016 •

edited

Loading