Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the Notification permission flow #1099

Closed
wants to merge 4 commits into from
Closed

Change the Notification permission flow #1099

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f313b4e
Change the Notification permission flow
arvindri2005 Dec 18, 2023
cfa1d51
string are placed in string resources files
arvindri2005 Dec 23, 2023
f193218
Change name of function to other name for better clarity
arvindri2005 Dec 23, 2023
b5b0095
Update ForYouScreenScreenshotTests.kt
arvindri2005 Dec 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions ...ndroidTest/kotlin/com/google/samples/apps/nowinandroid/feature/foryou/ForYouScreenTest.kt
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like all of the changes here are to make existing tests pass, could we add a new test for the updated flow?

Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,13 @@ class ForYouScreenTest {
onboardingUiState = OnboardingUiState.Loading,
feedState = NewsFeedUiState.Loading,
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
onTopicClick = {},
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onDeepLinkOpened = {},
Expand All @@ -84,9 +88,13 @@ class ForYouScreenTest {
onboardingUiState = OnboardingUiState.NotShown,
feedState = NewsFeedUiState.Success(emptyList()),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
onTopicClick = {},
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onDeepLinkOpened = {},
Expand Down Expand Up @@ -116,9 +124,13 @@ class ForYouScreenTest {
feed = emptyList(),
),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
onTopicClick = {},
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onDeepLinkOpened = {},
Expand Down Expand Up @@ -163,9 +175,13 @@ class ForYouScreenTest {
feed = emptyList(),
),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
onTopicClick = {},
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onDeepLinkOpened = {},
Expand Down Expand Up @@ -203,9 +219,13 @@ class ForYouScreenTest {
OnboardingUiState.Shown(topics = followableTopicTestData),
feedState = NewsFeedUiState.Loading,
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
onTopicClick = {},
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onDeepLinkOpened = {},
Expand All @@ -229,9 +249,13 @@ class ForYouScreenTest {
onboardingUiState = OnboardingUiState.NotShown,
feedState = NewsFeedUiState.Loading,
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
onTopicClick = {},
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onDeepLinkOpened = {},
Expand All @@ -256,9 +280,13 @@ class ForYouScreenTest {
feed = userNewsResourcesTestData,
),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
onTopicClick = {},
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onDeepLinkOpened = {},
Expand Down
85 changes: 83 additions & 2 deletions ...oryou/src/main/kotlin/com/google/samples/apps/nowinandroid/feature/foryou/ForYouScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,14 @@ import androidx.compose.foundation.lazy.staggeredgrid.StaggeredGridItemSpan
import androidx.compose.foundation.lazy.staggeredgrid.rememberLazyStaggeredGridState
import androidx.compose.foundation.shape.CornerSize
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material3.AlertDialog
import androidx.compose.material3.Icon
import androidx.compose.material3.MaterialTheme
import androidx.compose.material3.Surface
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
Expand Down Expand Up @@ -116,16 +118,22 @@ internal fun ForYouRoute(
val feedState by viewModel.feedState.collectAsStateWithLifecycle()
val isSyncing by viewModel.isSyncing.collectAsStateWithLifecycle()
val deepLinkedUserNewsResource by viewModel.deepLinkedNewsResource.collectAsStateWithLifecycle()
val showDialog by viewModel.dialogState.collectAsState()
val askNotificationPermission by viewModel.showNotificationPermission.collectAsState()

ForYouScreen(
isSyncing = isSyncing,
onboardingUiState = onboardingUiState,
feedState = feedState,
deepLinkedUserNewsResource = deepLinkedUserNewsResource,
askShowDialog = showDialog,
askNotificationPermission = askNotificationPermission,
onTopicCheckedChanged = viewModel::updateTopicSelection,
onDeepLinkOpened = viewModel::onDeepLinkOpened,
onTopicClick = onTopicClick,
saveFollowedTopics = viewModel::dismissOnboarding,
showNotificationPermissionDialog = viewModel::toggleDialog,
showNotificationPermission = viewModel::showNotificationPermission,
onNewsResourcesCheckedChanged = viewModel::updateNewsResourceSaved,
onNewsResourceViewed = { viewModel.setNewsResourceViewed(it, true) },
modifier = modifier,
Expand All @@ -138,10 +146,14 @@ internal fun ForYouScreen(
onboardingUiState: OnboardingUiState,
feedState: NewsFeedUiState,
deepLinkedUserNewsResource: UserNewsResource?,
askShowDialog: Boolean,
askNotificationPermission: Boolean,
onTopicCheckedChanged: (String, Boolean) -> Unit,
onTopicClick: (String) -> Unit,
onDeepLinkOpened: (String) -> Unit,
saveFollowedTopics: () -> Unit,
showNotificationPermissionDialog: () -> Unit,
showNotificationPermission: () -> Unit,
Copy link

Choose a reason for hiding this comment

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

I suggest renaming these to onNotificationPermissionDismiss and onNotificationPermissionConfirm alternatively

onNewsResourcesCheckedChanged: (String, Boolean) -> Unit,
onNewsResourceViewed: (String) -> Unit,
modifier: Modifier = Modifier,
Expand All @@ -160,6 +172,18 @@ internal fun ForYouScreen(
)
TrackScrollJank(scrollableState = state, stateName = "forYou:feed")

if(askShowDialog){
ShowDialog(
showNotificationPermissionDialog,
showNotificationPermission
)
}

if(askNotificationPermission){
NotificationPermissionEffect()
showNotificationPermission()
}

Box(
modifier = modifier
.fillMaxSize(),
Expand All @@ -177,6 +201,7 @@ internal fun ForYouScreen(
onboardingUiState = onboardingUiState,
onTopicCheckedChanged = onTopicCheckedChanged,
saveFollowedTopics = saveFollowedTopics,
askNotificationPermission = showNotificationPermissionDialog,
// Custom LayoutModifier to remove the enforced parent 16.dp contentPadding
// from the LazyVerticalGrid and enable edge-to-edge scrolling for this section
interestsItemModifier = Modifier.layout { measurable, constraints ->
Expand Down Expand Up @@ -244,7 +269,7 @@ internal fun ForYouScreen(
)
}
TrackScreenViewEvent(screenName = "ForYou")
NotificationPermissionEffect()
// NotificationPermissionEffect()
Copy link
Contributor

Choose a reason for hiding this comment

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

Before merging, commented out code should be removed entirely.

DeepLinkEffect(
deepLinkedUserNewsResource,
onDeepLinkOpened,
Expand All @@ -260,6 +285,7 @@ private fun LazyStaggeredGridScope.onboarding(
onboardingUiState: OnboardingUiState,
onTopicCheckedChanged: (String, Boolean) -> Unit,
saveFollowedTopics: () -> Unit,
askNotificationPermission: () -> Unit,
interestsItemModifier: Modifier = Modifier,
) {
when (onboardingUiState) {
Expand Down Expand Up @@ -298,7 +324,10 @@ private fun LazyStaggeredGridScope.onboarding(
modifier = Modifier.fillMaxWidth(),
) {
NiaButton(
onClick = saveFollowedTopics,
onClick = {
askNotificationPermission()
saveFollowedTopics()
},
enabled = onboardingUiState.isDismissable,
modifier = Modifier
.padding(horizontal = 24.dp)
Expand Down Expand Up @@ -444,6 +473,38 @@ fun TopicIcon(
)
}

@Composable
private fun ShowDialog(
Copy link

Choose a reason for hiding this comment

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

I suggest renaming this function to ShowNotificationPermissionDialog for better clarity.

onDismissRequest: ()-> Unit,
askNotificationPermission: () -> Unit
) {
AlertDialog(
onDismissRequest = onDismissRequest,
title = {
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?")
Copy link
Contributor

Choose a reason for hiding this comment

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

String resources should be placed in strings.xml

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arvindri2005 Please do not resolve comments before they have actually been resolved.

Copy link
Author

Choose a reason for hiding this comment

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

I have fixed this in the latest commit cfa1d51

},
confirmButton = {
NiaButton(
onClick = {
askNotificationPermission()
onDismissRequest()
},
) {
Text(text = "OK")
}
},
dismissButton = {
NiaButton(onClick = onDismissRequest ) {
Text(text ="Cancel")
Copy link

Choose a reason for hiding this comment

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

To maintain consistency and ease of localization, the "Ok" and the "Cancel" strings also should be included in the string file.

}
},
)
}


@Composable
@OptIn(ExperimentalPermissionsApi::class)
private fun NotificationPermissionEffect() {
Expand Down Expand Up @@ -516,8 +577,12 @@ fun ForYouScreenPopulatedFeed(
feed = userNewsResources,
),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onTopicClick = {},
Expand All @@ -542,8 +607,12 @@ fun ForYouScreenOfflinePopulatedFeed(
feed = userNewsResources,
),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onTopicClick = {},
Expand Down Expand Up @@ -571,8 +640,12 @@ fun ForYouScreenTopicSelection(
feed = userNewsResources,
),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onTopicClick = {},
Expand All @@ -592,8 +665,12 @@ fun ForYouScreenLoading() {
onboardingUiState = OnboardingUiState.Loading,
feedState = NewsFeedUiState.Loading,
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onTopicClick = {},
Expand All @@ -618,8 +695,12 @@ fun ForYouScreenPopulatedAndLoading(
feed = userNewsResources,
),
deepLinkedUserNewsResource = null,
askShowDialog = false,
askNotificationPermission = false,
onTopicCheckedChanged = { _, _ -> },
saveFollowedTopics = {},
showNotificationPermission = {},
showNotificationPermissionDialog = {},
onNewsResourcesCheckedChanged = { _, _ -> },
onNewsResourceViewed = {},
onTopicClick = {},
Expand Down
18 changes: 18 additions & 0 deletions ...ou/src/main/kotlin/com/google/samples/apps/nowinandroid/feature/foryou/ForYouViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import com.google.samples.apps.nowinandroid.core.ui.NewsFeedUiState
import com.google.samples.apps.nowinandroid.feature.foryou.navigation.LINKED_NEWS_RESOURCE_ID
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.combine
Expand Down Expand Up @@ -109,6 +110,23 @@ class ForYouViewModel @Inject constructor(
initialValue = OnboardingUiState.Loading,
)

private val _showNotificationPermission = MutableStateFlow(false)
val showNotificationPermission: StateFlow<Boolean> = _showNotificationPermission
private val _dialogState = MutableStateFlow(false)
val dialogState: StateFlow<Boolean> = _dialogState
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be named more descriptively? It's not clear which dialog this state is for.


fun toggleDialog() {
viewModelScope.launch {
_dialogState.emit(!_dialogState.value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

_dialogState.update {} is simpler

Copy link
Author

@arvindri2005 arvindri2005 Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
fun showNotificationPermission() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

viewModelScope.launch {
_showNotificationPermission.emit(!_showNotificationPermission.value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above 🙏

}


fun updateTopicSelection(topicId: String, isChecked: Boolean) {
viewModelScope.launch {
userDataRepository.setTopicIdFollowed(topicId, isChecked)
Expand Down
Loading