Change the Notification permission flow #1099
Conversation
Now Notification permission will be ask after knowing the interests of the user
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| Text(text = "Grant Notification Permission") | ||
| }, | ||
| text = { | ||
| Text("Would you like to be informed when new content is published on these topics, content is typically published every few days?") |
There was a problem hiding this comment.
String resources should be placed in strings.xml
There was a problem hiding this comment.
@arvindri2005 Please do not resolve comments before they have actually been resolved.
There was a problem hiding this comment.
I have fixed this in the latest commit cfa1d51
| viewModelScope.launch { | ||
| _dialogState.emit(!_dialogState.value) | ||
| } |
There was a problem hiding this comment.
_dialogState.update {} is simpler
There was a problem hiding this comment.
val showNotificationPermission = savedStateHandle.getStateFlow(SHOW_NOTIFICATION_PERMISSION, false)
val dialogState = savedStateHandle.getStateFlow(DIALOG_STATE, false)
fun toggleDialog() {
savedStateHandle[DIALOG_STATE] = !dialogState.value
}
fun showNotificationPermission() {
savedStateHandle[SHOW_NOTIFICATION_PERMISSION] = !showNotificationPermission.value
}
you want something Like this
| viewModelScope.launch { | ||
| _showNotificationPermission.emit(!_showNotificationPermission.value) | ||
| } |
| }, | ||
| dismissButton = { | ||
| NiaButton(onClick = onDismissRequest ) { | ||
| Text(text ="Cancel") |
There was a problem hiding this comment.
To maintain consistency and ease of localization, the "Ok" and the "Cancel" strings also should be included in the string file.
| @@ -444,6 +473,38 @@ fun TopicIcon( | |||
| ) | |||
| } | |||
|
|
|||
| @Composable | |||
| private fun ShowDialog( | |||
There was a problem hiding this comment.
I suggest renaming this function to ShowNotificationPermissionDialog for better clarity.
| showNotificationPermissionDialog: () -> Unit, | ||
| showNotificationPermission: () -> Unit, |
There was a problem hiding this comment.
I suggest renaming these to onNotificationPermissionDismiss and onNotificationPermissionConfirm alternatively
| _dialogState.update {!_dialogState.value} | ||
| } | ||
| } | ||
| fun showNotificationPermission() { |
There was a problem hiding this comment.
The name of this function is showNotificationPermission, but it looks like it will toggle whether we are asking for a notification permission. Can the the name of this function be updated, or have two different functions?
| private val _showNotificationPermission = MutableStateFlow(false) | ||
| val showNotificationPermission: StateFlow<Boolean> = _showNotificationPermission | ||
| private val _dialogState = MutableStateFlow(false) | ||
| val dialogState: StateFlow<Boolean> = _dialogState |
There was a problem hiding this comment.
Could this be named more descriptively? It's not clear which dialog this state is for.
| @@ -244,7 +269,7 @@ internal fun ForYouScreen( | |||
| ) | |||
| } | |||
| TrackScreenViewEvent(screenName = "ForYou") | |||
| NotificationPermissionEffect() | |||
| // NotificationPermissionEffect() | |||
There was a problem hiding this comment.
Before merging, commented out code should be removed entirely.
There was a problem hiding this comment.
It looks like all of the changes here are to make existing tests pass, could we add a new test for the updated flow?
| onTopicCheckedChanged = viewModel::updateTopicSelection, | ||
| onDeepLinkOpened = viewModel::onDeepLinkOpened, | ||
| onTopicClick = onTopicClick, | ||
| saveFollowedTopics = viewModel::dismissOnboarding, | ||
| onNotificationPermissionDismiss = viewModel::toggleNotificationPermissionDialog, |
There was a problem hiding this comment.
Similar to the comment in the viewmodel, this parameter is named dismiss, but it is used as a toggle (and called that way later). Could we change this to be two different functions to make the behavior more clear?
|
|
||
| if(askNotificationPermission){ | ||
| NotificationPermissionEffect() | ||
| onNotificationPermissionConfirm() |
There was a problem hiding this comment.
This is a side-effect causing function called directly in composition - this should likely be wrapped in a SideEffect or something similar.
|
Thank you for submitting this pull request. We have decided not to accept it at this time. |
Fixes #1095
Now Notification permission will be asked after knowing the interests of the user
WhatsApp.Video.2023-12-18.at.20.35.41_0476bb29.mp4