-
Notifications
You must be signed in to change notification settings - Fork 98
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
Flow coordinators #927
Flow coordinators #927
Conversation
Generated by 🚫 Danger Swift against 4a123ca |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #927 +/- ##
===========================================
- Coverage 48.24% 47.92% -0.32%
===========================================
Files 335 333 -2
Lines 20679 20556 -123
Branches 11326 11354 +28
===========================================
- Hits 9976 9852 -124
- Misses 10422 10425 +3
+ Partials 281 279 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor! 🎉
Left some comments/ideas inline.
Also noticed the navigation to room's settings using the contextual menu changed.
- Before: the room's settings were pushed on the Home Screen
- Now: the is the room's timeline pushed in between
To me it looks still good, but let's check everyone is happy.
Definitely, thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall seems like a decent change to me. We discussed offline about a possibility of only having handleAppRoute
as the public method to encourage the use of AppRoute
s as a single system for navigation throughout the app.
ElementX/Sources/FlowCoordinators/RoomFlowCoordinatorStateMachine.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/FlowCoordinators/RoomFlowCoordinatorStateMachine.swift
Outdated
Show resolved
Hide resolved
31e9bcb
to
ef29037
Compare
…d FlowCoordinator protocol Squashed commits: Add unit tests and move the state machine into the FlowCoordinator [bb68686] Replace the RoomFlowCoordinator's public interface with just `handleAppRoute` [0d9a4f8] Remove the navigationStackCoordinator dependency from the roomScreenCoordinator [4b5fbdf] Allow rooms to be selected from any other state [41dbd12] Move all missing coordinators to the RoomFlowCoordinator and state machines [f32431b] The UserSessionFlowCoordinator does not need to conform to the CoordinatorProtocol [0f07e87] Fix leaving a room dismissing the currently selected one when different [138385a] Rewind the navigation stack when re-selecting the same room (iPad) [0727eb9] Fix presenting different room details from the side menu on iPads [faf4cc6] Fix selecting the same room multiple times [fb3391d] Move room details presentation responsibility to the RoomFlowCoordinator. Fixed invitation flows. [fa2a68d] Rename RoomTimelineFlowCoordinator -> RoomFlowCoordinator [0c9c06b] Start moving things away from the RoomScreenCoordinator and into the RoomTimelineFlowCoordinator [86cbbdc] Introduce a RoomTimelineFlowCoordinator to deal with timeline related operations [9b2381b] Introduce the FlowCoordinatorProtocol
ef29037
to
9be50cb
Compare
…meline provider and other screens
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This PR is the first step towards introducing more flow coordinators, solidifying our navigation and fixing existing bugs, while making a whole class of potential problems easy to handle.
Fixes #918
Relates to #919
We've added a new RoomFlowCoordinator that is responsible for everything happening within a Room's context: