Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
## **Description** Within the scope of [Notifications Project](https://github.com/MetaMask/mobile-planning/issues/1495), this PR replaces libraries and approaches on how MetaMask handles notifications to our users. The adoption of this approach will pave the way for MM make usage of push notifications (back/foregrounds). REMINDER: Although this PR can be reviewed separately, triggering a NEW BUILD should be done together with [UI changes PR](#8961) so the user can have full control about push notifications. ## **Related issues** Fixes: [Jira Ticket](https://consensyssoftware.atlassian.net/browse/NOTIFY-466?atlOrigin=eyJpIjoiOWM0YTQ1YjVlN2UzNGFkZWIyZWY0M2ExZDE4ODE3NTMiLCJwIjoiaiJ9) ## **Manual testing steps** 1. Go to any token details page. 2. Submit a transaction and close the app. 3. Wait for the Push Notification and click on it. ## **Screenshots/Recordings** ### **After** https://github.com/MetaMask/metamask-mobile/assets/44679989/c64f88a3-1613-42a8-a076-1aecc266413a ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [x] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
|
Hi @Jonathansoufer can you link the figma file attached to this? |
There was a problem hiding this comment.
Left some comments on a few reviewed files. Since there are some pretty significant changes in this feature, I'd like to request to break this out into smaller scoped PRs. In addition to good practice, this suggested approach is inline with the team's mission in making PRs more reviewable + reducing chances of bugs. It will also result in a quicker turn around time and enable members from all teams to review the different chunks.
I'd recommend breaking it out in terms of dependencies and testability (unit testable). Here are some examples of how it can be broken out:
- component-library
- onboarding wizard
- boiler plate for actions and reducers
- third party libs
- image assets
- Notification screens (NotificationsView, NotificationsDetails, GasDetails, etc)
- translations
- notifications utils
- Lastly, hooking up all of the business logic so to connect all of the above
Could you also provide more context in the PR description? For example, could you include the issue number, Figma designs, steps and flows to test, and initial refinement details such as scope of work etc.
| // eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-commonjs | ||
| export const SAMPLE_BADGENOTIFICATIONS_PROPS: BadgeNotificationsProps = { | ||
| name: TEST_NOTIFICATIONS_ACTION, | ||
| iconName: TEST_RNOTIFICATIONS_ICON_NAME, |
|
|
||
| const BadgeBadgeNotificationsMeta = { | ||
| title: 'Component Library / Badges', | ||
| component: BadgeBadgeNotificationssComponent, |
| import { SAMPLE_BADGENOTIFICATIONS_PROPS } from './BadgeNotifications.constants'; | ||
| import { BadgeNotificationsProps } from './BadgeNotifications.types'; | ||
|
|
||
| const BadgeBadgeNotificationsMeta = { |
| }, | ||
| }, | ||
| }; | ||
| export default BadgeBadgeNotificationsMeta; |
| @@ -0,0 +1,34 @@ | |||
| # [BadgeNotifications](https://metamask-consensys.notion.site/Badge-Notifications-94a679c50cb446f4844dc624b4f74946) | |||
There was a problem hiding this comment.
Use Badge notification content
| @@ -0,0 +1,40 @@ | |||
| // Third party dependencies. | |||
| import React from 'react'; | |||
| import { shallow } from 'enzyme'; | |||
There was a problem hiding this comment.
Use react testing library instead
| @@ -0,0 +1,19 @@ | |||
| import React from 'react'; | |||
| import { shallow } from 'enzyme'; | |||
There was a problem hiding this comment.
Use react testing library instead
| ) => { | ||
| const handleCTAPress = () => { | ||
| navigation.navigate('Webview', { | ||
| screen: 'SimpleWebview', |
| <View style={styles.renderFCMContainer}> | ||
| <View style={styles.renderFCMCard}> | ||
| <Image | ||
| source={{ uri: notification.data.image.file.url }} |
There was a problem hiding this comment.
I suspect this may crash the app if url isn't defined. Is there a fallback view in the designs?
| <Button | ||
| variant={ButtonVariants.Secondary} | ||
| label={ | ||
| (notification.data?.link as unknown as { linkText?: string }) |
There was a problem hiding this comment.
Let's abstract this out into a variable since it's a complex condition
brianacnguyen
left a comment
There was a problem hiding this comment.
Can you add the figma link to the description?
Description
This PR implements all Notification flows
Manual testing steps
Screenshots/Recordings
After
Screen.Recording.2024-03-27.at.18.50.56.mov
Pre-merge author checklist
Pre-merge reviewer checklist