Kibana: [Meta] Replace all instances of Notifier with Toasts

Created on 25 Feb 2019  路  14Comments  路  Source: elastic/kibana

... By 7.1.0

The new platform offers a way to tap into the toast notification system to surface errors. This new system should be used over the legacy Notifier.

Don't do this:

screen shot 2019-02-25 at 13 00 29 pm

Do this:

screen shot 2019-02-25 at 13 04 32 pm

_Screenshots for visual representation only, error messages will differ_


There are roughly 28 files where Notifier is currently being used.

screen shot 2019-02-25 at 14 10 23 pm

Tagging teams that have instances of Notifier. When you've been able to convert all your files to the new toast system, you can check the box next to your team.

  • [ ] @elastic/kibana-app (#38116)
  • [ ] @elastic/logstash (#38116)
  • [ ] @elastic/ml-ui
  • [x] @elastic/kibana-security
Meta high hanging fruit

Most helpful comment

@cchaos The old notification system allows showing "more" about the error. We usually use that in case an exception was thrown, so the user is able to get the exception (and copy it for creating an issue possibly). The new toast notification system currently doesn't have a way for that. Also showing a stacktrace might need some larger width then a toast currently offer. What would be your suggestion around a design for handling that "more" case/showing stacktraces with the toast notifications?

All 14 comments

@cchaos The old notification system allows showing "more" about the error. We usually use that in case an exception was thrown, so the user is able to get the exception (and copy it for creating an issue possibly). The new toast notification system currently doesn't have a way for that. Also showing a stacktrace might need some larger width then a toast currently offer. What would be your suggestion around a design for handling that "more" case/showing stacktraces with the toast notifications?

@cchaos note that for ML, the changes required go beyond the x-pack/plugins/ml/public/components/full_time_range_selector/full_time_range_selector_service.js file listed as we also have our own message bar implementation, x-pack/plugins/ml/public/components/messagebar/messagebar_service.js, which uses ui/notify under the hood. This makes use of the 'show more' functionality mentioned by @timroes in https://github.com/elastic/kibana/issues/31943#issuecomment-467392483 to show further details such as stack traces e.g.

image

screen shot 2018-07-16 at 12 39 10

To switch the ML message bar service over to using EUI toasts, we would need some way of providing 'more' information, which would require greater space than the current toast allows.

There's been a long-standing open issue in EUI for this. I'll push this up to be discussed at our weekly.

Guidelines for this are coming in EUI. https://github.com/elastic/eui/pull/1611

In short, link off to modal with a button inside the toast.

Guideline merged. Example of how to handle it in gif above.

I'll build that pattern into the toastNotification service, so that every team can easily use this for presenting stack traces, so please feel free to wait for that to be finished before switching over in your application.

Thanks @timroes !

Unfortunately found a limitation in the EUI toast notification system, that currently prevents us from switching: https://github.com/elastic/eui/issues/1623

I'll nevertheless prepare a PR with the API to show errors, but we need this to be solved beforehand in EUI before we can actually switch over errors to toast notifications.

We have now introduced a toastNotifications.addError method to legacy toastNotifications service and the new platform service. I setup a PR that rewrites most of the places that use Notifier to the new method (https://github.com/elastic/kibana/pull/38116).

The only area mostly open is @elastic/kibana-security. You had a lot of places where you used Notifier also not just for error showing, so I would appreciate if your team could open a separate PR moving from Notifier to toastNotifications.

@timroes I'll start looking Security's usage of Notifier

Security's use of Notifier has been converted to toasts: https://github.com/elastic/kibana/pull/38260

All Notifier usages have been removed from Kibana now. I'll remove the Notifier class itself in a separate PR.

Reopening since it seems I missed, that ML is still using Notifier in a couple of places. @elastic/ml-ui could you please remove your usage of notify imported from ui/notify and replace it instead by the appropriate toastNotifications.

Once that's done I can remove the Notifier service.

cc @timroes - should be good to go for ML https://github.com/elastic/kibana/pull/41191

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ctindel picture ctindel  路  81Comments

Alex-Ikanow picture Alex-Ikanow  路  364Comments

passkey1510 picture passkey1510  路  96Comments

doubret picture doubret  路  105Comments

stormpython picture stormpython  路  74Comments