Skip to content

Conversation

sjchmiela
Copy link
Contributor

@sjchmiela sjchmiela commented Jan 23, 2020

Why

We want to have a way to build notifications based on some requests, eg. from presentNotificationAsync({…}), or from a data property of a RemoteMessage.

How

NotificationBuilderFactory provides NotificationBuilders capable of configuring NotificationCompat.Builder instances according to configuration passed in with setRemoteMessage(RemoteMessage).

This is the class we will extend (and eventually refactor if it gets too complicated, eg. into the Modifiers scheme) to support more fields from the notifications object to set Intents, links, number etc.

Note: Builder uses NotificationChannel.DEFAULT_CHANNEL_ID at the moment, which results in not showing the notification. This module will be integrated with NotificationChannelsManager once it lands.

excalidraw-202012311929-8

More complete architecture diagram (receiving + handling + building)

excalidraw-202012311929-10

Test Plan

I have confirmed these classes work as expected in a more complex PR.

@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-buildingnotifications branch 2 times, most recently from c663647 to 089b190 Compare January 24, 2020 09:36
@sjchmiela sjchmiela marked this pull request as ready for review January 24, 2020 09:41
@sjchmiela sjchmiela requested a review from mczernek as a code owner January 24, 2020 09:41
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-buildingnotifications branch 2 times, most recently from f1be211 to d9cf860 Compare January 24, 2020 16:04
@sjchmiela sjchmiela requested a review from ide January 24, 2020 16:06
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-buildingnotifications branch 3 times, most recently from 001e271 to 3f61daf Compare January 31, 2020 14:10
Copy link
Contributor

@esamelson esamelson left a comment

Choose a reason for hiding this comment

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

this looks good, i have a few comments about functionality that would be great to add, but if they are already addressed in one of your newer PRs i haven't looked at yet, i apologize!!

@tsapeta tsapeta linked an issue Feb 5, 2020 that may be closed by this pull request
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-buildingnotifications branch from cc1e2a1 to 108dbdb Compare February 11, 2020 15:45
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-buildingnotifications branch from 108dbdb to 13751cd Compare February 12, 2020 15:14
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-buildingnotifications branch from 13751cd to 25135fd Compare February 13, 2020 09:42
@sjchmiela sjchmiela merged commit 596463b into master Feb 13, 2020
@sjchmiela sjchmiela deleted the @sjchmiela/expo-notifications-buildingnotifications branch February 13, 2020 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract Notifications to unimodule
4 participants