Improve Notifications

[cjcenizal]

Related to:

• Possible issue with histogram popup time #600
• Surface relationships between Saved Objects when saving #5947
• Timeout on message bar could cause confusion if window not in focus #7409
• Give user more control over notifications #7912

Problem

1. Our single Notifier service is used for all notifications, regardless of the content or context of the notification. This inadequately serves our UI/UX needs, since notifications can be classified into different types with different requirements.
2. This also creates a code maintenance problem, since we end up piling more and more functionality into the Notifier service, creating unmodular and kitchen-sink-ish code.

Solution

I met with @alt74 and worked out some solutions from a UI/UX standpoint. Then I met with @tsullivan and we discussed some goals for our Notifier code and tossed around implementation considerations and ideas.

Types of notifications

Not all notifications are the same. For example, we currently expose a fatal method on the Notifier class, which tells the user Kibana is broken and blocks interaction with the UI, despite this being drastically different than the rest of the notifications, which all stack at the top of the screen.

Jurgen, Tim, and I came up with the following notification types.

Local notifications

Local notifications can come in many forms, but they are all characterized as inline components placed in close proximity to the context to which it pertains. An example in Kibana would be a message that looks like a Bootstrap alert that displays above the Users table after you've added a user, and says "User successfully added."

These wireframes are examples of other local notifications; a progress bar and an alert.

Soft notifications

Sometimes you may want to alert the user to a non-critical change in the application. If the user sees it, s/he will find the information useful, but if the user misses it (by being away from the computer or paying attention to another window), then it's no big deal. In this case, you could display a brief toast in the corner of the screen that disappears after a few seconds, or after the user navigates to a different app.

This is probably the closest component to our existing notifications implementation.

Hard notifications

If you require a decision or data from your user, you can present a Hard notification, which interrupts the user flow and blocks interaction with the Kibana UI. This component could be called a Dialogue, a Non-dismissable Modal, a Confirmation, or an OverPanel. It can be used in lieu of window.confirm, and can respond to smaller screens by taking over the entire screen for improved mobile UX.

Fatal notification

For when Kibana enters an unusable state and requires a restart.

Logged notifications

Some notifications represent activity that should be logged so the user can review at any time. At the same time, we don't want to compel the user to review unseen Logged notifications -- the user will know when s/he wants to look at the log; we don't need to bug them the way Slack notifications do.

These wireframes display how the Activity Log icon in the side bar can "whisper" to the user by briefly displaying a small, truncated version of the notification message, but no counter or badge indicating unseen notifications. The bunch of lines on the right is a poor representation of this view, which would be a list of all the Logged notifications.

Engineering considerations

There are some changes we can make to our existing notification system to make it easier to implement the above systems. And implementing some of the above systems will also simplify our existing notification system (the Notifier code).

Reduce dependencies on Notifier

We should reduce the number of dependencies on the Notifier service. This will make it easier to refactor and remove unnecessary functionality. We can do this in a few ways, easiest first:

1. Extract the fatal method into its own service.
2. Make local notifications wherever possible, even if they are simple Bootstrap-type Alerts.
3. Create a Hard notification component, which will simply prompt for confirmation. We can use this in lieu of window.confirm, mediated via the safeConfirm service.

Refactoring Notifier

Our Notifier class is currently instantiated by the consuming code, but this seems like an architectural mistake. The methods it surfaces (error, warning, info) are factory creation methods, so it would be better suited as a singleton that creates Notification instances.

Implementation ideas

Uncoupling the creation of a notification from how it's presented

When a module needs to create a notification, it should only need to dispatch an event:

// monitoring/public/index.js
Notifier.show(NotificationTypes.PHONE_HOME_DATA_SHARING, {
  onAccept: () => features.update(PHONE_HOME_FEATURE, true),
  onReject: () => features.update(PHONE_HOME_FEATURE, true),
});

We will have already registered the appropriate directive to display when this event is received:

// monitoring/public/index.js
Notifier.register(NotificationTypes.PHONE_HOME_DATA_SHARING, '<phone-home-data-sharing on-accept={onAccept} on-reject={onReject}></phone-home-data-sharing>');

Then our Notifications directive can render whatever notifications are being shown by Notifier:

// kibana/some-main-view-directive.js
for (var i = 0; i < Notifier.length; i++) {
  const notification = Notifier.get(i);
  const el = $compile(notification.template)(notification.scope);
  $element.parent().append( el );
}

This way our implementation of the notification is uncoupled from wherever it's being used. We can reuse it in multiple different places and also write unit tests for it specifically. This also allows us to create notifications using regular Angular directives instead of homegrown pseudo-directives.

[Bargs]

Make local notifications wherever possible, even if they are simple Bootstrap-type Alerts.

How do you feel about creating a directive for these? We actually have a number of local notifications in the app but there's no consistent way to create them. The only thing most of them have in common is that they use the bootstrap alert class. I think that's partially why there's so much use of the notification service, because it's lower friction than rolling your own local alerts.

[thomasneirynck]

Should we also consider notifications that are only displayed once?

a welcome toast, a suggestion for a configuration change, ... you only want to show a single time, the first time a user uses the app. Some toasts have a "do not remind me" dismissal button.

Should this be captured by the Notifier, or should the client manage this?

[cjcenizal]

@Bargs Great point. I agree that we should create directives for local notifications.

@thomasneirynck I think it really depends on the UI/UX we want for such a notification. Our current notification system can accommodate your use case, as long as it makes sense to present it as a banner at the top of the screen. But if we want to present it in a different way (e.g. a full-screen modal), then we should use a different component and build a new system to display it.

[tsullivan]

Re: Soft Notifications and "This is probably the closest component to our existing notifications implementation."

I could see that as being true for the self-dismissing info notifications. But I think the Soft Notification brings more goodness. What I really don't like about the current "info" notification is that it pushes the entire page down to accommodate the height of the notification. The lifetime of the notification is also managed by the client, so it's not always consistent. Whenever they start to stack up it gets really annoying and confusing and ugly (sorry, but I feel strongly about this).

I really hope we can figure out how to make Soft Notifications more digestible when there is more than 1 on the screen, and I think animations will help a lot. When a Soft Notification goes away, it should probably fade out, and if there is another one above it, the new one should slide down until it also fades away. Of course, we should try to leverage Local Notification if possible and only use Soft Notification if it's really necessary and that should help the stack not getting too large.

[Hard notifications] can respond to smaller screens by taking over the entire screen for improved mobile UX.

Glad you included this point. This excites me!

The methods it surfaces (error, warning, info) are factory creation methods, so it would be better suited as a singleton that creates Notification instances.

And there is a banner and a custom methods as well. Note that the current framework also surfaces the add method (this is what all the "type" methods call internally), and there is at least one example of code that uses that. I believe add should be a private method, if possible, because it opens the door for inconsistency.

How do you feel about creating a directive for [local notifications]? We actually have a number of local notifications in the app but there's no consistent way to create them.

++ for the directive. There shouldn't be a hoop to jump through for any of these features. It is unfortunate that we don't have an easy way to create an "alert," and therefore Local Notifications are pretty much going to be a new feature.

Should we also consider notifications that are only displayed once?

I am for having the client manage this, instead of doing some thing like pass a flag to the Notifieri service to have it manage showing the notification once. If the notification should only be shown once, chances are that there will be a need to bring it back later, by changing an option in Management. The client should own the behavior of having the notification shown once, and the options available in Management as well.

This is so great! I can already imagine a bunch of interactions that will get a huge boost from better notifications.

[cjcenizal]

Per a conversation with the QA team, @LeeDr mentioned that functional tests have a hard time knowing when async actions are done, e.g. loading, saving, or deleting something. I think that improving the way we surface UI state via notifications will ameliorate this problem.

For example, if there's an inline notification in a table that says "row saved", then the functional test can just look for that element via test subject selector (robust) instead of searching for a toast with a specific string (brittle).

[cjcenizal]

Largely addressed by #15749.