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

feat: notification centre #3973

Merged
merged 6 commits into from Jun 17, 2022
Merged

feat: notification centre #3973

merged 6 commits into from Jun 17, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Jun 14, 2022

What it solves

Resolves #3950

How this PR fixes it

Notifications are now cached locally and are accessible via a notification centre. It is opened by clicking the bell in the header.

Notifications are unread if the user does not manually close the notification. Unread notifications are displayed at the top of the notification centre and marked as read when closing the notification centre if they were visible in the list.

Note: 'actionable' notifications are not implemented yet, hence why the "Required actions" section of the notification centre/notification actions are not present. These will be implemented as part of the #3951 epic.

How to test it

Dispatch notifications via requesting a transaction signature, accessing a Safe that requires an update, etc. and observe that they are accessible in the notification centre.

Screenshots

notification centre

@iamacook iamacook self-assigned this Jun 14, 2022
@iamacook iamacook linked an issue Jun 14, 2022 that may be closed by this pull request
4 tasks
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Jun 14, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@github-actions
Copy link

Deployment links

Mainnet     🟣 Polygon     🟠 Rinkeby

@coveralls
Copy link

coveralls commented Jun 14, 2022

Pull Request Test Coverage Report for Build 2514004709

  • 37 of 85 (43.53%) changed or added relevant lines in 8 files are covered.
  • 138 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.2%) to 36.804%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/AppLayout/Header/components/Notifications/NotificationItem.tsx 5 6 83.33%
src/components/AppLayout/Header/components/Layout.tsx 0 2 0.0%
src/logic/hooks/useNotifier.tsx 2 6 33.33%
src/components/AppLayout/Header/components/Notifications/NotificationList.tsx 3 8 37.5%
src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx 0 5 0.0%
src/logic/notifications/store/notifications.ts 8 16 50.0%
src/components/AppLayout/Header/components/Notifications/index.tsx 18 41 43.9%
Files with Coverage Reduction New Missed Lines %
src/logic/hooks/useNotifier.tsx 1 62.96%
src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx 2 0%
src/logic/wallets/utils/unstoppableDomains.ts 3 14.29%
src/components/App/ReceiveModal.tsx 4 0%
src/utils/googleTagManager.ts 13 64.29%
src/components/Dashboard/SafeApps/index.tsx 18 0%
src/routes/CreateSafePage/components/SafeCreationProcess.tsx 41 7.14%
src/routes/opening/index.tsx 56 7.04%
Totals Coverage Status
Change from base Build 2487795864: 0.2%
Covered Lines: 3893
Relevant Lines: 9605

💛 - Coveralls

* feat: add notification bell in header

* fix: remove unused prop
* feat: add notification bell in header

* fix: remove unused prop

* feat: notification popper

* fix: mark notifications read on popper close

* feat: add scrollbar

* fix: chronological sort + increase button size

* fix: lint + remove padding

* fix: update type, split components + simplify sort

* fix: cleanup clickaways

* fix: add return type

* fix: use popper offset

* fix: close poppers relevant to notifications

* fix: ordering + don't read pending notifications

* fix: remove comment

* fix: clickable area of bell
@iamacook iamacook marked this pull request as ready for review June 15, 2022 10:28
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@francovenica
Copy link
Contributor

1 - Notification about a new version for the safe:
The first time you see it (by visiting a safe < 1.3.0) the notification pops up just fine. But after that every time you visit that safe the notification pops up and hided right away.
I've checked in prod and this behavior doesn't happen

A safe 1.0.0 for testing: rin:0xFA8A0Ec18381eA8edB018F62Cb1dfF6B641Bf221
test1

2 - This issue happens also in prod. If it can be fixed here great, if not I'll create a new ticket for it.

Notification of "Sign the tx" when using a spending limit never go away on their own.
Give an owner spending limit of any ERC20
Login with that owner
Create a tx sending funds and use the spending limit feature
When the MM pop up shows also the notification of "sing the tx" shows up
That notification never goes away on its own
test2

The rest of the notifications looks good to me

@iamacook
Copy link
Member Author

Thanks for testing this @francovenica. Both issues should now be fixed.

@liliya-soroka
Copy link
Member

Verified:

  1. 2 issues from Franco are fixed
  2. Also the next changes were implemented and verified:
  • if the notification message is closed it's displayed as read in the notification center
  • if the notification message is not closed by the user the un-read circle appears on the bell as soon as the message is hidden

@iamacook iamacook merged commit 5db53ae into dev Jun 17, 2022
@iamacook iamacook deleted the 3950-epic-notification-centre branch June 17, 2022 10:32
@github-actions github-actions bot locked and limited conversation to collaborators Jun 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Epic] Notification Centre
5 participants