Set state of {ush Notifs Switcher based on notifications permissions and prevent changing it if notifications permissions are not granted. Finally deep-link user to application settings if the try to change it when notifications permissions are not granted. It must be noted however that whether notifications are enabled in general is not reflected in threadInfo.currentUser.subscriptions in database.
Details
Build app, disable/enalbe notifications in settings and check whether switcher state is correct and whether deep-link alert is shown when notifications are disabled.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-863
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
62 ↗ | (On Diff #11342) | disabled prop on Switch |
In the future, please re-request review when you have a question to make sure the diff goes to my queue. I am often multiple days behind on Phabricator emails, but I try to stay up-to-date on my diff review queue
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
62 ↗ | (On Diff #11342) | How are we then going to deep-link user to notification settings? If we disable the switcher then onValueChange callback will not be fired. |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
62 ↗ | (On Diff #11342) | Wrap in TouchableWithoutFeedback (discussed over chat) |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
62 ↗ | (On Diff #11342) | @ashoat For me the main question isn't how we can detect an interaction, but if we really want to do that. The issue is that for a user it might be really difficult to discover that clicking a disabled control has some effect. Usually users ignore disabled components and clicking them isn't intuitive. In my opinion, it would be better to have an explanation in some discoverable place, e.g. i icon next to the disabled component - after clicking it we would show a message. |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
62 ↗ | (On Diff #11342) | I still have a concern. You mentioned setting disabled just after this comment: |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
62 ↗ | (On Diff #11342) | @marcinwasowicz – the idea is that the control will appear enabled or disabled based on whether notifs are enabled for this thread for this user (which does not depend on iOS / Android notifs permission – it is simply a setting stored in MySQL). Separately, we would disable the control if the iOS / Android notifs permissions are not enabled. So it could be disabled & true, or disabled & false. @palys-swm – good point! I think your idea of an i icon seems like a good one. Either way, I think when the control goes from false -> true, if notif permissions are not enabled we should Alert the user and deeplink them into the settings. |
Rebase & Introduce UI changes: when notifs are disabled the Switcher is disabled and the 'i' icon is rendered. Tapping 'i' results in alert linking to notification settings. An icon is not rendered when notifs are enabled.
Please update the summary and the test plan and include a screenshot with the current approach.
Edit: it seems that the images were shared before I submitted my comments. That's great!
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
59 | TouchableWithoutFeedback should generally be avoided - it doesn't give a user any feedback. It was used initially here so that we can get an event from a disabled control - but that was just a workaround. In this case we should probably use TouchableOpacity. |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
63 | What do you think about decreasing the contrast by making this a shade of gray? |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
63 | Yes. Currently the icon is highlighted just like it was the most important component on this screen. I think we can modify it and give it the same color the text has. |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
102–103 ↗ | (On Diff #11982) | Maybe we should give a more specific message here that explains that since notifications are not enabled on this device, you can't modify push notif settings? |
117–119 ↗ | (On Diff #11982) | Instead of having this defined here, I think it is better to have hasPushPermissions in Props, and to directly select hasPushPermissions using useSelector in ConnectedThreadSettingsPushNotifs: const hasPushPermissions = useSelector(state => state.deviceToken !== null); |
Use selector passed in props to detect whether app has push permissions. Modify Alert content.
Going forward, please be much more thoughtful about copy. Your approach right now feels very lazy... copy is critical, and it's a constant frustration of mine that nobody on the team besides me seems to take it seriously
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
100 ↗ | (On Diff #12037) | In push-handler.js, we clearly use different copy for iOS and Android. Why did you get rid of the Android message? Was it inaccurate, perhaps? |
102–103 ↗ | (On Diff #12037) | You have a single word threadPlease here. Did you test this? Let me just rewrite it for you, that will probably be faster. I think it would be good to have several different texts depending on iOS vs. Android and on this.state.currentValue. If it's iOS and notifs are enabled, we can say:
If it's iOS and notifs are disabled, we can say:
(Note the use of Unicode →, and the removal of the period at the end) If it's Android, we can probably just use the copy in push-handler.js. |
106 ↗ | (On Diff #12037) | This is a really long text for a button... let's go with just "Go to settings" |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
63 ↗ | (On Diff #12095) | I'm not sure if we should use icon from @expo/vector-icons here. We don't use this icon pack anywhere in mobile app. We even don't have dependency to that package in package.json - it's dependency of expo package. |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
56–68 ↗ | (On Diff #12095) | Let's avoid functions that implicitly return undefined like this, unless they have no return statements. So a function that does not use return is okay... but if the function does use return something anywhere, then we should avoid implicit return or even the keyword return;, but instead should explicitly annotate return undefined;. Additionally, we can invert the condition to return indentation. You should always try to do that |
63 ↗ | (On Diff #12095) | Interesting, I didn't realize we even had @expo/vector-icons installed. Turns out it got added in the unimodules -> expo migration. I think it is basically the same thing as react-native-vector-icons... we should dedup them. I created a backlog task to track. As for the question of which icon to use – can you upload screenshots of each so we can compare? |
74 ↗ | (On Diff #12095) | Not super clear to me why you defined a function for this... why not just use the basic formulation: let node = undefined; if (condition) { node = ( <JSX /> ); } |
108 ↗ | (On Diff #12095) | Can you use a Unicode apostrophe character here? ’ |
This is icon from expo/vector-icons (current implementation):
This is 'info-circle' icon from SWMansionIcon that was suggested by Jacek (<SWMansionIcon name="info-circle" size={25} color="gray" />)
@ashoat, @def-au1t please, let me know which one should be used for this diff.
I think it makes sense to put the info icon to the immediate right of "Push notifs" instead of by the toggle?
Something like:
The idea is that it's explaining why the control is disabled, not explaining the nature of the option itself
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
163 ↗ | (On Diff #12346) | In D3564 at line 386 we explicitly set device token to null in case notifications permissions are denied. I am not JavaScript expert so I understand it might still make sense to check for undefined here, but perhaps it is not necessary given the fact it is set to null somewhere else. |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
163 ↗ | (On Diff #12346) | In AppState we define deviceToken as ?string which means https://flow.org/try/#0DYUwLgBAHhBcEH4DOYBOBLAdgcwNwCgYBeCAV0wBMQAzLECg4iTU4YAoA that both null and undefined can be assigned to it. It is safer to not assume that D3564 is the only place which changes this value. Checking both values here will make the code more maintainable.
It's not something JS-specific, but the less we assume in the code, the more maintainable it becomes. Or in other words, it's harder to break something by accident. |
native/chat/settings/thread-settings-push-notifs.react.js | ||
---|---|---|
123 ↗ | (On Diff #12346) | This change indeed works the same but I receive the following flow error: () => Promise<void> Open app settings. See https://reactnative.dev/docs/linking.html#opensettings Cannot get `Linking.openSettings` because property `openSettings` [1] cannot be unbound from the context [2] where it was defined.Flow(method-unbinding) |