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.

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

Tree Upload Reminder Dialog #1081

Merged
merged 6 commits into from
Apr 5, 2023

Conversation

CalebKL
Copy link
Contributor

@CalebKL CalebKL commented Mar 17, 2023

This PR adds Alert Dialog to users who have planted trees above 2000 and reminds them to upload the trees.
Fixes on Issue #1057

Copy link
Contributor

@Elforama Elforama left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this. There are some required changes, but it looks good overall.

Please keep formatting in mind while working, try to match the style in the file.

Comment on lines 69 to 70
title = "Upload Trees Soon",
message ="You have over 2000 trees, please upload as soon as you can.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Text that the user sees should always use string resources to allow translations.

Dashboard(
state = state,
snackBar = snackBar,
onSyncClicked = { viewModel.sync() },
onOrgClicked = { navController.navigate(NavRoute.Org.route) },
onCaptureClicked = { navController.navigate(NavRoute.UserSelect.route) },
onCaptureClicked = {
if (state.totalTreesToSync >=2000){
Copy link
Contributor

Choose a reason for hiding this comment

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

We can move this check into a boolean method on the state object that returns true or false

import androidx.compose.ui.unit.sp

@Composable
fun DisplayAlertDialog(
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a dialog method we can reuse that has a special style that matches the app. Please use that instead

CalebKL added 2 commits March 22, 2023 09:49
-Added treesToSyncThreshold on the viewmodel and updated state
Copy link
Contributor

@Elforama Elforama left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, it's looking a lot better. Can you please cleanup the last few things before I merge it?

@@ -63,13 +61,29 @@ fun DashboardScreen(
val state by viewModel.state.observeAsState(DashboardState())
val snackBar by viewModel.snackBar.observeAsState()
val navController = LocalNavHostController.current
var dialogOpened by remember { mutableStateOf(false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

dialogOpened doesn't seem to be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -36,6 +36,7 @@ data class DashboardState(
val totalTreesToSync: Int = 0,
val isOrgButtonEnabled: Boolean = false,
val showUnreadMessageNotification: Boolean = false,
val totalTreesToSyncThreshold:Boolean = false
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be renamed to something more descriptive like "showTreeSyncReminderDialog"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Elforama Elforama merged commit 59dcb74 into Greenstand:master Apr 5, 2023
@CalebKL CalebKL deleted the TreeUploadReminderDialog branch April 5, 2023 04:27
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.

2 participants