MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.84k stars 4.83k forks source link

Add snaps notifications #13553

Closed rekmarks closed 2 years ago

rekmarks commented 2 years ago

Once we added the ability for snaps to create notifications, we should implement these notifications in MetaMask.

We are likely to implement notifications that appear inside MetaMask itself, and for those we will need designs. These notifications should:

The other option for notifications is to give snaps access to native browser notifications, if in an attenuated way. It's unclear if this is something we should or will pursue at this time.

Montoya commented 2 years ago

Curious to know what the user experience will be when the MetaMask popup / full UI is not in focus, whether using built-in or native browser notifications. Will the user see the notification or will they miss it? Also, if native browser notifications are used, will that require the user to make an extra step in order to give permissions in the native browser UI?

rekmarks commented 2 years ago

Hey team! Please add your planning poker estimate with ZenHub @Gudahtt @hmalik88 @ritave

rekmarks commented 2 years ago

@Montoya, we'd make the notifications persist in the MetaMask UI until dismissed by the user, or perhaps timed out if we can convince ourselves that the user saw them (perhaps by the page that they're displayed on in our UI being active).

For the native browser notification, the extension already has that permission, so there will be no extra prompt.

holantonela commented 2 years ago

We will re-use existing MM components to allow developers to prompt notifications inside MM via Snaps.

Ideally, developers should be able to define:

  1. Style: Which type of notification is going to be triggered? Does it require user input? We have two types of notifications currently available: a. Popover b. Actionable Message

  2. Type: What is the status of this notification? Each UI component has different available states, aka type. The Actionable Message can be Default, Warning, Success.

  3. Context: Where is this notification going to be shown? Developers will be able to define where the notification should appear. I'd start with a few options. I'd leave Confirmation screens for a second iteration. a. Home b. Token Details

It will be the first batch of UI components we will expose to developers via Snaps! I expect that we can share feedback to the Design System team to help document their work on improving the consistency/flexibility of these components.

Also, ideally, this implementation does not break (or can be easily fixed) after they re-shape this UI.

What do y'all think?

cc @georgewrmarshall @Akatori-Design

holantonela commented 2 years ago

Do we want to make explicit/visible that the notification is triggered by a Snap and not MM itself? A helper tooltip can make the job.

Montoya commented 2 years ago

Tooltip sounds like a good approach. I do not want to over-optimize for Snaps. Ideally Snaps can behave like first-class functionality in MetaMask until we feel like we need to differentiate them from native MetaMask functionality for some reason.

Gudahtt commented 2 years ago

We already have the "home notification"/"system notification" system, which shows messages on the Home screen that can persist until dismissed, and can include custom content and actions. It also includes a system for stacking them, and hiding additional messages behind an expand/collapse button if there are too many. This is already the established place for showing messages that aren't tied to a specific UI context like a confirmation screen. This seems like a better place to start than designing a new notification system from scratch that would be in the same place and face many of the same challenges.

If we're going to build a new system, then a lot of follow up questions come to mind. Are these blocking, or can they be ignored by the user? How can we ensure they don't obscure important parts of the Home screen? How do we handle showing multiple messages at once? Would we absorb the home notifications into this system, or somehow show both at once?

holantonela commented 2 years ago

Just to be clear, I'm not trying to create a new notification system. I'm just pointing on which components we could rely to make it more extensible and easy to maintain.

If we want to attach this feature with our Home Notification System, we can do it! It will help also to iterate what we consider on the current implementation. Which component it is using @Gudahtt? Do we have documentation somewhere to understand which variables we can offer to developers?

georgewrmarshall commented 2 years ago

@holantonela regarding the notification components I think they need a bit of work before we can release them to the community. The api for the ActionableMessage component is quite messy. I'd like to clean it up or possibly deprecate it in favour of a new component. I think the Popover is in better shape but I would still like to have a look at the component api before giving it the green light.

Gudahtt commented 2 years ago

Which component it is using @Gudahtt? Do we have documentation somewhere to understand which variables we can offer to developers?

There is no documentation for system notifications because it's an internal system. We have only used it ourselves 7 times. The exact details of what it can do today don't really matter, are subject to change, and would not be directly exposed via the snaps API anyway.

It is currently using its own component. It was built before the two components you referenced.

It will be the first batch of UI components we will expose to developers via Snaps!

I think they need a bit of work before we can release them to the community.

I did not think this issue involved custom UI. I had assumed that we would be treating this similarly to custom confirmations, i.e. it would be our UI, just with custom contents defined by the snap. And perhaps other configurable options like buttons, actions, styles, etc. The snap authors will not be directly building UI with our components.

holantonela commented 2 years ago

I did not think this issue involved custom UI. I had assumed that we would be treating this similarly to custom confirmations, i.e. it would be our UI, just with custom contents defined by the snap. And perhaps other configurable options like buttons, actions, styles, etc. The snap authors will not be directly building UI with our components.

Yeah. That is the point. When I said "expose" I meant "allow developers to push content in the UI component".

holantonela commented 2 years ago

After our Design Sync, we refined some of the requirements for this feature on V1

holantonela commented 2 years ago

WIP https://www.figma.com/file/2QWFaRYpa9SDZRCwFlW17j/Snap-Notifications?node-id=0%3A297

holantonela commented 2 years ago

Hi, the latest UI is here https://www.figma.com/file/2QWFaRYpa9SDZRCwFlW17j/Snap-Notifications?node-id=0%3A297

Update on design requirements:

holantonela commented 2 years ago

Also, some thoughts about how notifications are marked as read

Notifications are marked as read if the user:
- interacts with the notification (click a link) 
- interacts with the notification list (scroll)
- click on `mark all as read` button
FrederikBolding commented 2 years ago

Closing via #14605