GT-2465 resume lesson modal #2352
Conversation
…into GT-2465-Resume-Lesson-Modal
| @@ -0,0 +1,15 @@ | |||
| // | |||
| // GetResumeLessonProgressModalInterfaceStringsRepositoryInterface.swift | |||
There was a problem hiding this comment.
Hey @levieggertcru, how do you feel about the naming convention making some names super long? Was debating to take out the word "progress", but still gonna be pretty long either way 🤔
There was a problem hiding this comment.
Hey @rachaelblue I know some names get super long. I know we suffix a lot of key names such as UseCase, DomainModel, RepositoryInterface, etc.
And some names get descriptive based on feature so there isn't collisions with other features. (If we could namespace that would help here).
I think we could start omitting Repository from the interface naming.
Maybe GetResumeLessonInterfaceStringsInterface or even GetResumeLessonStringsInterface. A tiny bit shorter.
| let buttonWidth = (modalWidth - totalSpaceAroundButtons) / 2 | ||
|
|
||
| ZStack { | ||
| if #available(iOS 15.0, *) { |
There was a problem hiding this comment.
Also @levieggertcru, do you think we can bump the minimum deployment target to 15?
There was a problem hiding this comment.
With iOS 18 out I feel like we could. Let me check the analytics data and see where user count is at. Would like to confirm with godtools team as well.
There was a problem hiding this comment.
We got the go ahead to bump to iOS 15 so feel free to make that change in this PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2352 +/- ##
===========================================
- Coverage 95.92% 95.90% -0.02%
===========================================
Files 62 62
Lines 5323 5323
===========================================
- Hits 5106 5105 -1
- Misses 217 218 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Hey @rachaelblue looking good. The only changes I'm seeing is with the coordinator (Flow).
Remember that all navigation decisions will need to go through the coordinator.
Some other things to mention:
-
I'm not crazy about this step name presentResumeLessonModal since it's describing the navigation to perform. There's a couple steps in FlowStep I need to look into changing as well that contain show in the name. Steps really should describe some action or event that has taken place and then the coordinator decides what navigation to perform when that action or event takes place.
-
As best as possible I want to avoid injecting closures into ViewModels for navigation steps. Instead should use the FlowDelegate.
Changes requested:
-
Instead of LessonViewModel.swift making the decision to present the Resume Lesson Modal, this decision should happen in the flow. I would say LessonFlow can perform this decision. So the LessonFlow would first check to see if Resume Lesson Modal should be presented before performing navigation to the lesson.
-
Remove closures in ResumeLessonProgressModalViewModel.swift and instead perform navigation steps on the FlowDelegate. If start over is tapped from the Resume Lesson Modal, then the flow will start the lesson from the beginning. If continue is tapped from the Resume Lesson Modal, then the flow will start the lesson from the last recorded progress.
...es/UserLessonProgress/Presentation/ResumeLessonProgressModal/ResumeLessonProgressModal.swift
Outdated
Show resolved
Hide resolved
…into GT-2465-Resume-Lesson-Modal # Conflicts: # godtools/Base.lproj/Localizable.strings
|
Hey @levieggertcru, I originally was presenting the modal from the AppFlow and then navigating to the lesson after the user made a selection, but there's a slight delay when the lesson loads/presents, and it looked awkward to have the modal disappear and see no lesson for a second. So my solution was to have the lesson and modal present nearly simultaneously so that the lesson is already behind the modal when the user makes a selection, and the page number gets updated if needed. Visually this looks better, but does add the complication that the lesson opens before we know whether the user wants to resume or restart. I tried making that happen from the lesson flow instead, but it does feel a little weird for the flow to be holding onto the viewModel in order to get page info/update the page. Let me know what you think. |
|
Hey @rachaelblue what if you push the LessonView onto the navigation stack with animated false and then dismiss the resume lesson modal? Is there still some delay in the LessonView itself? I know that view waits for the layout constraints to complete before doing any navigation. |
|
Hey @levieggertcru, that did work better to present LessonView without animation before dismissing the modal. Felt a little weird moving some initialization around in the LessonFlow now, so let me know what you think. Thanks! |
No description provided.