feat: [FC-0047] calendar synchronization design #439
Conversation
|
Thanks for the pull request, @IvanStepanok! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
| if !course.active { | ||
| Text(ProfileLocalization.CoursesToSync.inactive) | ||
| .font(Theme.Fonts.labelSmall) | ||
| .foregroundStyle(Theme.Colors.textPrimary.opacity(course.active ? 1 : 0.8)) |
There was a problem hiding this comment.
Checking course.active ? seems redundant as we are in the if !course.active { block
| @@ -69,6 +69,7 @@ public struct StyledButton: View { | |||
| } | |||
| Spacer() | |||
| } | |||
| .padding(.horizontal, 20) | |||
There was a problem hiding this comment.
please be careful with changes to the base views - it can produce issues like this one
OpenEdX/Router.swift
Outdated
| @@ -701,6 +701,28 @@ public class Router: AuthorizationRouter, | |||
| navigationController.pushViewController(controller, animated: true) | |||
| } | |||
|
|
|||
| public func showDatesAndCalendar() { | |||
| let vm = DatesAndCalendarViewModel( | |||
There was a problem hiding this comment.
Maybe will be better to register this viewModel in DI (ScreenAssembly)?
Also name viewModel is more informative than just vm
OpenEdX/Router.swift
Outdated
| navigationController.pushViewController(controller, animated: true) | ||
| } | ||
|
|
||
| public func showSyncCalendarOptions(viewModel: DatesAndCalendarViewModel) { |
There was a problem hiding this comment.
could we use here also viewModel from DI instead of send it as parameter?
OpenEdX/Router.swift
Outdated
| navigationController.pushViewController(controller, animated: true) | ||
| } | ||
|
|
||
| public func showCoursesToSync(viewModel: DatesAndCalendarViewModel) { |
| } | ||
|
|
||
| func openAppSettings() { | ||
| if let url = URL(string: UIApplication.openSettingsURLString) { |
There was a problem hiding this comment.
Maybe put the conditions separated by a comma instead of having two if constructs?
| if isHorizontal { | ||
| ScrollView { | ||
| content | ||
| .frame(maxWidth: 400) |
There was a problem hiding this comment.
modifications here and for content in else block are same - maybe we need to move them in content view?
| } | ||
| }) | ||
| .tint(Theme.Colors.accentColor) | ||
| // CustomToggle(isOn: $toggle) |
There was a problem hiding this comment.
will this customToggle be used?
There was a problem hiding this comment.
Not at the moment, but it will be included in the next scope of work. Thanks for checking! 😊
| Image(systemName: "chevron.right") | ||
| } | ||
| }) | ||
| .accessibilityIdentifier("courses_to_Sync_cell") |
|
|
||
| public init(router: ProfileRouter) { | ||
| self.router = router | ||
| self.calendarNameHint = (Bundle.main.applicationName ?? "") + " Course Dates" |
There was a problem hiding this comment.
perhaps " Course Dates" should be localized too?
volodymyr-chekyrta
changed the title
|
|
||
| StyledButton(ProfileLocalization.CalendarSync.button, action: { | ||
| viewModel.requestCalendarPermission() | ||
| }, horizontalPadding: true) |
There was a problem hiding this comment.
could we please follow code-style 'each parameter on own line' here?
| } | ||
|
|
||
| // MARK: - Navigation and Title | ||
| private var navigationAndTitle: some View { |
There was a problem hiding this comment.
Could you please attach created ticket here to be sure we wont miss this unification in the future?
remove old errorAlertView variables
|
@IvanStepanok 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Quick Preview
Screen.Recording.2024-05-20.at.15.32.35.mp4
Pay attention⚠️
This is just the design of the views, without any logic other than getting a request to access the calendar.
I'm using the "Use relative dates" switch to demonstrate the "calendar reconnect" process.
The "Use relative dates" switch will be implemented in the next scope.
enhancement request:
#443