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

Add snaps notifications #13553

Closed
rekmarks opened this issue Feb 8, 2022 · 16 comments
Closed

Add snaps notifications #13553

rekmarks opened this issue Feb 8, 2022 · 16 comments

Comments

@rekmarks
Copy link
Member

rekmarks commented Feb 8, 2022

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:

  • be dismissable.
  • appear on the home screen in the popup and/or the full screen UI.
  • add to the notification badge count on the browser action icon.

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
Copy link
Contributor

Montoya commented Feb 8, 2022

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
Copy link
Member Author

rekmarks commented Feb 9, 2022

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

@rekmarks
Copy link
Member Author

@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.

@FrederikBolding FrederikBolding self-assigned this Mar 7, 2022
@holantonela
Copy link

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
Copy link

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
Copy link
Contributor

Montoya commented Mar 8, 2022

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
Copy link
Member

Gudahtt commented Mar 8, 2022

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
Copy link

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
Copy link
Contributor

georgewrmarshall commented Mar 8, 2022

@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
Copy link
Member

Gudahtt commented Mar 8, 2022

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
Copy link

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
Copy link

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

  • Notifications are going to follow just one type of component (toast)
  • Users should be able to identify that a notification is coming from a Snap
  • Users should be able to read persistent notifications in a /notifications list kind of fashion
  • Users should be able to discover that a notification has been triggered even if the MM UI is closed
  • Buttons are not available
  • Notifications should be able to stack or get paged

@holantonela
Copy link

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

Update on design requirements:

  • Notifications are going to follow just one type of component (toast)
  • Users should be able to identify that a notification is coming from a Snap ✅
  • Users should be able to read persistent notifications in a /notifications list kind of fashion ✅
  • Users should be able to discover that a notification has been triggered even if the MM UI is closed ✅
  • Buttons are not available but links are ✅
  • Notifications should be able to stack or get paged ✅
  • Notifications should be able to get accessed via a menu link ✅

@holantonela
Copy link

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
Copy link
Member

Closing via #14605

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

7 participants