Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear All button and counter badge for the EuiGlobalToastList component #6945

Closed
delanni opened this issue Jul 13, 2023 · 10 comments
Closed

Comments

@delanni
Copy link

delanni commented Jul 13, 2023

Is your feature request related to a problem? Please describe.
The problem is not a developer problem, but it's a common, and frustrating issue users are facing. It's around overloading them with toast messages. See the longer threads here: elastic/kibana#116686, and here: elastic/kibana#161482

We agreed, we'd add a close all button to the notification container. However, it is not easy, and very error prone to implement this outside of EUI, and expect the button to be in the right place (because the EuiGlobalToastList is an absolutely placed/fixed component on the screen).

We've also agreed that toasts whose content is identical to a toast that is already on the screen will be combined into a single toast. We will show a badge on the top right concern of the toast, displaying the number of times that message is repeated

Describe the solution you'd like
We'd like a button to close all displayed toasts. An idea is to place this button just below all the toast. For the badge, we suggest the use of a EuiNotificationBadge in size m and subdued color.

image

Describe alternatives you've considered
For the Clear all button, we've considered:

  • a button in a toast message - it's a bit ugly looking (unnecessary toast-notification-like look), and a bit more convoluted to manage as a virtual toast-message
  • a button outside the EuiGlobalToastList - this is hard to place to the right place

For the badge, we've considered:
image

  • a EuiNotificationBadge in the toast's title, aligned vertically
  • a EuiNotificationBadge in the toast's title, aligned top

Additional context
elastic/kibana#116686
elastic/kibana#161482

@JasonStoltz
Copy link
Member

JasonStoltz commented Jul 13, 2023

Thank you @delanni. The team will discuss this early next week and try to get it prioritized. We'll also seek input from design. Do you have a desired timeline for this?

@delanni
Copy link
Author

delanni commented Jul 14, 2023

I don't think it's urgent. I think we can still deliver some user value without this improvement just by deduplicating toasts, we can add the button later on. So I don't think we have a timeline.

@ryankeairns
Copy link
Contributor

I mentioned this in the Kibana issue, but Clint had some feedback around how this will look for very long titles. With the badge at the end of the text, it will both move around and could appear at the extreme right end next to the close icon.

This is rather subjective but perhaps we place it in front, instead:

CleanShot 2023-07-20 at 08 57 55@2x

@JasonStoltz
Copy link
Member

An accessibility consideration: This button will need to be easy to navigate to from the same context as the toasts -- so being able to tab through toasts to the "Clear All" button.

@ryankeairns @andreadelrio Have ya'll reviewed this from a design standpoint?

@1Copenut
Copy link
Contributor

I wanted to add two additional a11y considerations:

  1. The Clear all button background needs a 3:1 or greater contrast ratio to adjacent background colors. The example made me think of this; the light blue button might not meet that threshold against a light gray background.
  2. We'll need accessible labels for the Clear all button and the number blocks. Accessible labels could be something like "Clear all toast messages" and "Title of toast. This message appears 12 times" or "Title of toast. 12 duplicated messages."

@JasonStoltz
Copy link
Member

@petrklapka @delanni We identified this as a "Medium" priority in our grooming meeting, which means it's not likely to get worked on in the near future. Please let us know if you believe this warrants a higher priority, or if you need a particular timeline for this request.

@andreadelrio andreadelrio changed the title Dismiss/Close all button for the EuiGlobalToastList component Clear All button and counter for the EuiGlobalToastList component Jul 25, 2023
@andreadelrio andreadelrio changed the title Clear All button and counter for the EuiGlobalToastList component Clear All button and counter badge for the EuiGlobalToastList component Jul 25, 2023
delanni added a commit to elastic/kibana that referenced this issue Jul 27, 2023
#161738)

## Summary
This PR addresses the occasional toast-floods/toast-storms with a simple
catch mechanism: deduplicate/group toasts by their broad alikeness,
their text and title.

This implementation plugs in to the `global_toast_list.tsx` in Kibana's
GlobalToastList component, capturing updates on the toast update stream,
and collapses toasts before passing them further to the downstream EUI
Toast list react components.

The core idea is to not display notifications directly, but to keep the
toast notifications apart from their visual representation. This way, we
can represent more notifications with one toast on our end, if we group
rightly. The only issue then, is to clean up everything nicely when it's
time. For this we're also exporting a mapping that shows which toast ID
represents which grouped toasts.

I also changed the type `ToastInputFields` to accept rendered react
components as title/text - this will prevent attempts to unmount react
components from elements that are not displayed, thus causing React
warnings.

The close-all button is an EUI feature, which we've started discussing
[here](elastic/eui#6945), probably not part of
this PR.

## What toasts get merged?
The exact merging strategy was not settled, and it's kind of a valve,
where we trade off potential detail lost in toasts for the prevented
noise in the toast floods. The current strategy is as folows:
```
 * These toasts will be merged:
 *  - where title and text are strings, and the same (1)
 *  - where titles are the same, and texts are missing (2)
 *  - where titles are the same, and the text's mount function is the same string (3)
 *  - where titles are missing, but the texts are the same string (4)
``` 
The base merge case is `(1) & (2)`, after some discussion with @Dosant
we decided to include (3) as a compromise, where we're still merging
somewhat similar toasts, and extend the merging to `ToastsApi::addError`
where all error toasts have a MountPoint as their text. We understand
this might hide some details (e.g.: same titles, but very slightly
different MountPoints as their text) but realistically this shouldn't
really happen.

The ultimate future improvement will be (as suggested in the comments by
@jloleysens) a sort of identifier to the toast, based on which we can
group without fear of losing information. But this will require more
work on all the call-sites.
 
Relates to: #161482 


![1ca12f39-75af-4d24-8906-9f27fad33c45](https://github.com/elastic/kibana/assets/4738868/b4578f2e-756d-40d0-9d24-fdffe8b9c724)


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this issue Aug 1, 2023
elastic#161738)

## Summary
This PR addresses the occasional toast-floods/toast-storms with a simple
catch mechanism: deduplicate/group toasts by their broad alikeness,
their text and title.

This implementation plugs in to the `global_toast_list.tsx` in Kibana's
GlobalToastList component, capturing updates on the toast update stream,
and collapses toasts before passing them further to the downstream EUI
Toast list react components.

The core idea is to not display notifications directly, but to keep the
toast notifications apart from their visual representation. This way, we
can represent more notifications with one toast on our end, if we group
rightly. The only issue then, is to clean up everything nicely when it's
time. For this we're also exporting a mapping that shows which toast ID
represents which grouped toasts.

I also changed the type `ToastInputFields` to accept rendered react
components as title/text - this will prevent attempts to unmount react
components from elements that are not displayed, thus causing React
warnings.

The close-all button is an EUI feature, which we've started discussing
[here](elastic/eui#6945), probably not part of
this PR.

## What toasts get merged?
The exact merging strategy was not settled, and it's kind of a valve,
where we trade off potential detail lost in toasts for the prevented
noise in the toast floods. The current strategy is as folows:
```
 * These toasts will be merged:
 *  - where title and text are strings, and the same (1)
 *  - where titles are the same, and texts are missing (2)
 *  - where titles are the same, and the text's mount function is the same string (3)
 *  - where titles are missing, but the texts are the same string (4)
``` 
The base merge case is `(1) & (2)`, after some discussion with @Dosant
we decided to include (3) as a compromise, where we're still merging
somewhat similar toasts, and extend the merging to `ToastsApi::addError`
where all error toasts have a MountPoint as their text. We understand
this might hide some details (e.g.: same titles, but very slightly
different MountPoints as their text) but realistically this shouldn't
really happen.

The ultimate future improvement will be (as suggested in the comments by
@jloleysens) a sort of identifier to the toast, based on which we can
group without fear of losing information. But this will require more
work on all the call-sites.
 
Relates to: elastic#161482 


![1ca12f39-75af-4d24-8906-9f27fad33c45](https://github.com/elastic/kibana/assets/4738868/b4578f2e-756d-40d0-9d24-fdffe8b9c724)


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@eokoneyo
Copy link
Contributor

Hi @JasonStoltz! I'm looking into the "clear All" button requirement of this issue, to accommodate another requirement for toast improvements in kibana (see here elastic/kibana#162646), I've put together a PoC (code is available here #7111) that takes into account couple of considerations already raised about implementing the "close all" button. I'd be curious to know if this fits into the direction the team would like to tackle including the clear all button.

@eokoneyo
Copy link
Contributor

Update: the implementation to include a clear all button in EUI got merged and landed in version 88.3.0, that still leaves the second part of the request; adding a counter badge, but for what it's worth, an implementation for this has been shipped within Kibana.

@JasonStoltz
Copy link
Member

@eokoneyo Great, thanks! Should we close this issue?

@eokoneyo
Copy link
Contributor

@eokoneyo Great, thanks! Should we close this issue?

Yes let's close it, the feature request was opened to fulfil requirements within Kibana that have been catered to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants