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

[MBL-18013][Student][Parent] Refactor reminder logic #2628

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

domonkosadam
Copy link
Contributor

Test plan:

  • Assignment without due date:
    • Previously users couldn't set reminder for assignments without due date. Now you can set reminder for these assignments as well.
  • Assignment with due date:
    • Users can set reminder from the new date time picker dialog.

refs: MBL-18013
affects: Student
release note: Reminders can be set for assignments without due date.

Screenshots

BeforeAfter

Checklist

  • Run E2E test suite
  • Tested in dark mode
  • Tested in light mode

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@domonkosadam domonkosadam marked this pull request as draft November 12, 2024 10:44
@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@domonkosadam domonkosadam marked this pull request as ready for review November 18, 2024 12:11
@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@tamaskozmer tamaskozmer self-requested a review November 18, 2024 12:44
@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Native Parent Install Page

@inst-danger
Copy link
Contributor

Student Install Page

Copy link
Contributor

@tamaskozmer tamaskozmer left a comment

Choose a reason for hiding this comment

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

QA findings:

  • Date picker dialog buttons are not styled
  • It would look better if the text was always alligned with the icon regardless of the line count. (screenshot attached)
Screenshot 2024-11-18 at 14 24 45

@@ -160,4 +165,25 @@ class ApplicationModule {
): OfflineAnalyticsManager {
return OfflineAnalyticsManager(context, analytics, pageViewUtils, apiPrefs, dateTimeProvider, featureFlagProvider)
}

@Provides
fun provideDateTimePicker(): DateTimePicker {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we providing these in the Application scope?

onReminderSelected(choices[which])
dialog.dismiss()
private fun showCreateReminderDialog(context: Context) {
viewModel.assignment?.let { assignment ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic belongs to the ViewModel. It would be nice to keep the unidirectional data flow also and not request data from the ViewModel.

@@ -451,7 +468,7 @@ class AssignmentDetailsViewModel @Inject constructor(
quizDetails = quizViewViewData,
attemptsViewData = attemptsViewData,
hasDraft = hasDraft,
showReminders = assignment.dueDate?.after(Date()).orDefault(),
showReminders = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this will always be true with the new logic. Maybe we don't need this condition anymore.

import kotlinx.coroutines.flow.callbackFlow
import java.util.Calendar

class DateTimePicker {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have some extensions for Date and Time pickers in the compose/Utils.kt file. Maybe you can take a look and see if that can be reused.

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.

3 participants