This differential uses new native API to unify thread settings push notifs alerts on iOS and Android 13+. Older Androids remain unchanged.
Details
Details
Build the app for Android 13. Deny notifications permission. Go to thread settings of any thread and tap "i" icon next to disabled switcher. Ensure that alerts look the same as on iOS except the path to notifications settings. Repeat for older Android. Ensure that behaviour remains
unchanged.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-3306
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
It is a visual diff so I attach how new alerts look on Android 13:
I think that we will probably need to change the way those arrows look on Android but I need an opinion and guidance from someone who is expert in visual appearance of the app.
| native/chat/settings/thread-settings-push-notifs.react.js | ||
|---|---|---|
| 102 ↗ | (On Diff #23934) | await is a very important keyword, and syntax tricks like this make it really hard for the reader to tell if an await is dispatched. When calling await, please make sure it always look like this: [left side of expression] = await [right side of expression]; Eg. here you can do: let platformRequestsPermission; if (Platform.OS !== 'android') { platformRequestsPermission = true; } else { platformRequestsPermission = await CommAndroidNotifications.canRequestNotificationsPermissionFromUser(); } Separately, since this is an Android-specific API I think it would make more sense to check if Platform.OS is 'android', just in case we ever use react-native-web or react-native-windows or something. |
| 102 ↗ | (On Diff #23934) | I honestly think it would be fine to just check Platform.Version here, and it would improve performance. @marcin what do you think of that? |
| native/chat/settings/thread-settings-push-notifs.react.js | ||
|---|---|---|
| 102 ↗ | (On Diff #23934) | CommAndroidNotifications.canRequestNotificationsPermissionFromUser() is probably very fast since it just checks SDK_INT. Personally I disagree that Platform.Version check is going to be better. The actual reason I introduced CommAndroidNotifications.canRequestNotificationsPermissionFromUser() method was to avoid directly checking for particular system version. I consider it more idiomatic and understandable for other developers. However if you are worried about the performance of this method - since it returns a promise - I would suggest changing it to a constant exported to JS. Something like: CommAndroidNotifications.CAN_REQUEST_NOTIF_PERMISSION_FRON_USER. This way we would be fast and idiomatic. Nevertheless I wouldn't expect CommAndroidNotifications.canRequestNotificationsPermissionFromUser() to be harmful for performance in this case. |
Comment Actions
Refactor code to make more readable check for ability to request notifications permission
| native/chat/settings/thread-settings-push-notifs.react.js | ||
|---|---|---|
| 102 ↗ | (On Diff #23934) | That's fair, I don't feel strongly |