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

Jump to bottom

ALTAPPS-1069: Shared add reminders time to notifications onboarding #786

Merged
merged 10 commits into from
Dec 15, 2023
Merged

ALTAPPS-1069: Shared add reminders time to notifications onboarding #786

merged 10 commits into from
Dec 15, 2023

Conversation

ivan-magda
Copy link
Member

YouTrack Issues:
#ALTAPPS-1069

Checklist

Before Code Review:

  • Fields "Assignees, Labels, Milestone" are filled in the pull request;
  • New analytics events are documented;
  • All checks have been passed;
  • Changes have been checked locally.

Description

  1. Deletes daily study reminders business logic from StepCompletionFeature
  2. Adds ability to select daily study reminders start hour on NotificationsOnboardingFeature

@ivan-magda ivan-magda added this to the 1.45 milestone Dec 13, 2023
@ivan-magda ivan-magda self-assigned this Dec 13, 2023
@github-actions github-actions bot added shared Shared module task ios iOS module task android Android module task labels Dec 13, 2023
ivan-magda
ivan-magda commented Dec 13, 2023
View reviewed changes
...n/org/hyperskill/app/notifications_onboarding/presentation/NotificationsOnboardingReducer.kt
Comment on lines +36 to +38
InternalAction.SaveDailyStudyRemindersIntervalStartHour(
startHour = state.dailyStudyRemindersStartHour
),
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, after clicking on the "Allow notifications" button, daily study reminders start hour interval will be set silently (without observing loading status and error state, see InternalAction.SaveDailyStudyRemindersIntervalStartHour).
I decided to make it so, because I don't like the behavior of multiple modals / loading indicators one after each other (system dialog of requesting notification permission, loading / error indicators)

cc @XanderZhu @vladkash

Copy link
Member Author

Choose a reason for hiding this comment

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

ALTAPPS-1079

@ivan-magda ivan-magda added the awaiting review Pull Request is awaiting code reviews label Dec 13, 2023
@ivan-magda ivan-magda requested a review from XanderZhu December 13, 2023 17:06
XanderZhu
XanderZhu approved these changes Dec 14, 2023
View reviewed changes
...n/org/hyperskill/app/notifications_onboarding/presentation/NotificationsOnboardingFeature.kt Outdated
internal data class State(val dailyStudyRemindersStartHour: Int)

data class ViewState(
val dailyStudyRemindersStartHour: Int,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need dailyStudyRemindersStartHour in the viewState?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be in NotificationsOnboardingFeature.Action.ViewAction.ShowDailyStudyRemindersIntervalStartHourPickerModal to select initial / current row in picker:

Simulator Screenshot - iPhone 15 Pro - 2023-12-15 at 11 17 57

@XanderZhu XanderZhu added ready to pull Pull Request is ready to merge and removed awaiting review Pull Request is awaiting code reviews labels Dec 14, 2023
@ivan-magda ivan-magda merged commit dab5f4a into develop Dec 15, 2023
9 checks passed
@ivan-magda ivan-magda deleted the feature/ALTAPPS-1069/shared-add-reminders-time-to-notification-onboarding branch December 15, 2023 04:40
@ivan-magda ivan-magda removed the ready to pull Pull Request is ready to merge label Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XanderZhu XanderZhu XanderZhu approved these changes

Assignees

@ivan-magda ivan-magda

Labels
android Android module task ios iOS module task shared Shared module task
Projects
None yet
Milestone
1.45
Development

Successfully merging this pull request may close these issues.
2 participants
@ivan-magda @XanderZhu