feat(Mobile): add notifications opt-In screen #4837
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Branch preview✅ Deploy successful! Storybook: |
Coverage (43%)
|
compojoom
reviewed
Jan 27, 2025
Comment on lines
6
to
15
| jest.mock('@notifee/react-native', () => ({ | ||
| AndroidImportance: { | ||
| NONE: 0, | ||
| MIN: 1, | ||
| LOW: 2, | ||
| DEFAULT: 3, | ||
| HIGH: 4, | ||
| }, | ||
| })) | ||
|
|
There was a problem hiding this comment.
I guess that we should move this to a global mock as you have it also in the Onboarding.container.test.tsx?
compojoom
reviewed
Jan 27, 2025
| @@ -17,6 +18,9 @@ const styles = StyleSheet.create({ | |||
| signing: { | |||
| height: Math.abs(windowHeight * 0.3), | |||
| }, | |||
| notifications: { | |||
| height: Math.abs(windowHeight * 0.32), | |||
There was a problem hiding this comment.
what's up with those magic contants here? For signing it is 0.3, here it is 0.32? Why are we multiplying with that? How does it look on a smaller/bigger device?
There was a problem hiding this comment.
rolledback since the whole implementation wasn't supposed to be on onboarding flow.
compojoom
reviewed
Jan 27, 2025
| @@ -55,6 +57,11 @@ const useNotifications = (): NotificationsProps => { | |||
| if (isAppNotificationEnabled) { | |||
| dispatch(toggleAppNotifications(false)) | |||
| } | |||
|
|
|||
| if (isOnboarding) { | |||
| router.navigate('/(tabs)') | |||
There was a problem hiding this comment.
I think that we should be using replace instead of navigate. With navigate on devices with a hardware back button you would be able to go bath to the onboarding screeen. Where with replace you won't be.
|
@Jonathansoufer could you please adjust the PR title as per our guidelines? |
Jonathansoufer
changed the title
Jonathansoufer
changed the title
compojoom
reviewed
Jan 30, 2025
compojoom
reviewed
Jan 30, 2025
Comment on lines
+25
to
+41
| const { isAppNotificationEnabled, promptAttempts } = useNotifications() | ||
| const dispatch = useAppDispatch() | ||
| const router = useRouter() | ||
|
|
||
| /* | ||
| * If the user has not enabled notifications and has not been prompted to enable them, | ||
| * redirect to the opt-in screen | ||
| * */ | ||
|
|
||
| const shouldShowOptIn = !isAppNotificationEnabled && !promptAttempts | ||
|
|
||
| useEffect(() => { | ||
| if (shouldShowOptIn) { | ||
| dispatch(updatePromptAttempts(1)) | ||
| router.navigate('/notifications-opt-in') | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
This is weird. The AssetsContainer should not care about notifications.
Get some inspiration from here:
https://docs.expo.dev/router/reference/authentication/
https://reactnavigation.org/docs/auth-flow
I think that the _layout.tsx should be the place where we decide what first screen we show to the user.
There was a problem hiding this comment.
I completely get your point here, but we don't wanna spam our users before the Onboarding, and if we use _layout, this takes place before anything. The other alternative would be calling it at HomeScreen, wdyt?
There was a problem hiding this comment.
When the user clicks on "get started/continue" in the onboarding flow we should set a flag "onboarding_done": true. Next time the user starts we should not show the onboarding screens. So we will be deciding in the _layout.tsx which screen to show anyway.
Jonathansoufer
changed the title
compojoom
approved these changes
Jan 31, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What it solves
This ticket implements the notifications onboarding screen, enabling users to toggle the feature ON or deciding to leave for later. Its important to mention though that after three rejected attempts the user will not be prompt again within a month.
P.S: Although out of the scope of this PR, it was implemented a reusable FloatingContainer which is intent to be used to group and stick elements on the bottom of the screen. It has also a awareness of keyboard elements on those cases when keyboard is necessary to proceed with an action.
Resolves #
How this PR fixes it
How to test it
Screenshots
Checklist