Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Notifications improvements #158

Closed
lukasschor opened this issue Sep 5, 2019 · 1 comment
Closed

Notifications improvements #158

lukasschor opened this issue Sep 5, 2019 · 1 comment
Assignees
Labels
Enhancement ✨ Minor Improvement / changes to existing functionality Minor Needs to be fixed within the next 1-3 public releases.

Comments

@lukasschor
Copy link
Member

lukasschor commented Sep 5, 2019

List of non-sticky and sticky notifications: https://docs.google.com/spreadsheets/d/1McBmkEKDUIEFec0pvsz46bXpxL5j9dC74wdfLhVw_bY/edit#gid=0

Most of the notifications are already implemented but need to be checked if they behave according to the requirements in the spreadsheet.

TO-DO:
Notificacions are done, but at some point Metamask will stop reloading dApps when the network is changed in the extension. We should store the keys of the notifications related with the wallet connection and network (there is no need to store the restof them) in redux, so we can close them when some events (provider update) happen.
The basic structure is done, but not working.

@lukasschor lukasschor self-assigned this Sep 5, 2019
@lukasschor lukasschor added this to the Safe for Teams v1.0.0 (Devcon) milestone Sep 6, 2019
@lukasschor lukasschor added the Minor Needs to be fixed within the next 1-3 public releases. label Sep 6, 2019
@lukasschor lukasschor modified the milestones: Safe for Teams v1.0.0 (Devcon), Safe for Teams v1.5.0 (Devcon), Safe for Teams v1.1.0 (Devcon - Nice-to have’s) Sep 6, 2019
@germartinez germartinez changed the title Notifications Notifications improvements Sep 30, 2019
@lukasschor lukasschor added the Enhancement ✨ Minor Improvement / changes to existing functionality label Oct 1, 2019
@lukasschor
Copy link
Member Author

lukasschor commented Oct 25, 2019

@germartinez Users are confused that the "Transaction / Owner Change / ... Pending" Notifications have a green checkmark. This suggests the action is completed, but in fact, the transaction is still being processed. Could we use a loading spinner instead of the checkmark? If that's not easy to implement, we should replace the checkmark with the yellow "!" icon.

Also, I would change the wording for all policy changes (owner changes, threshold changes) no matter if 1-oo-n or m-oo-n threshold to say "Policy change pending". The reason for this is that the "More confirmations required..." part is confusing as this very confirmation is still pending and only after that, it becomes even relevant that other confirmations are needed. Also, the type of policy change is not that relevant as the user just initiated it and knows what it is about. So it's more important that the notification looks recognizable thuse the same wording across policy changes.

There are some other improvements that we could do in a future iteration of the notifications, but I think this ones are easy fixes that we should include already now.

germartinez added a commit that referenced this issue Nov 13, 2019
@mmv08 mmv08 closed this as completed Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement ✨ Minor Improvement / changes to existing functionality Minor Needs to be fixed within the next 1-3 public releases.
Projects
None yet
Development

No branches or pull requests

2 participants