[Spaces] M10.8 Browsing users in a space #4682 #4742
Conversation
- Added MVVM classes - roughly integrated `RoomParticipantsViewController` to the MVVM
- Added navigation to member detail page
- Present space members screen from people tab if space has been selected
There was a problem hiding this comment.
- The
adminlabel doesn't seem to be right-aligned, so it starts in a random location depending on character length - In dark theme, the search empty state is a light image
- Empty state text has
space nameas actual text, should be name of currently visisble space
…_a_space # Conflicts: # Riot/Modules/SideMenu/SideMenuCoordinator.swift
- Renamed coordinators
- Update after design review
- Update after design review
|
@gileluard Looks good, I just realised one thing when searching members that I didn't really think of sooner. Seeing the big image appear here feels a bit strange, as the keyboard is up it's not clear what it means. Those empty states are useful when everything is visisble: image and text. The text underneath is much more helpful. Could we either just remove the image, or replace with same no results design for rooms, but with members info instead
|
…_a_space # Conflicts: # Riot/Modules/SideMenu/SideMenuCoordinator.swift
- Update after design review
| @@ -870,6 +879,19 @@ - (void)pushViewController:(UIViewController*)viewController | |||
| } | |||
| } | |||
|
|
|||
| - (void)showDetailFor:(MXRoomMember* _Nonnull)member from:(UIView* _Nullable)sourceView { | |||
There was a problem hiding this comment.
sourceView parameter seems to be not used here.
There was a problem hiding this comment.
It's not used here indeed but it's used by SpaceMemberListViewController that inherits from RoomParticipantsViewController
Riot/Modules/Spaces/SpaceMembers/MemberDetail/SpaceMemberDetailCoordinator.swift
Outdated
Show resolved
Hide resolved
Riot/Modules/Spaces/SpaceMembers/MemberDetail/SpaceMemberDetailViewModel.swift
Outdated
Show resolved
Hide resolved
- Update after code review
| let spaceMembersCoordinator = SpaceMembersCoordinator(parameters: parameters) | ||
| spaceMembersCoordinator.delegate = self | ||
| let presentable = spaceMembersCoordinator.toPresentable() | ||
| presentable.presentationController?.delegate = self |
There was a problem hiding this comment.
Even if it's fine to let it here I think it can be interesting to put presentable.presentationController?.delegate = self inside SpaceMembersCoordinator. We will add this to the file template. And add a delegate method for this case. It will centralise this edge case and avoid to forget it for other screens.
| @@ -131,7 +131,7 @@ final class AppCoordinator: NSObject, AppCoordinatorType { | |||
|
|
|||
| private func addSideMenu() { | |||
| let appInfo = AppInfo.current | |||
| let coordinatorParameters = SideMenuCoordinatorParameters(appNavigator: self.appNavigator, userSessionsService: self.userSessionsService, appInfo: appInfo) | |||
| let coordinatorParameters = SideMenuCoordinatorParameters(appNavigator: self.appNavigator, appCoordinator: self, userSessionsService: self.userSessionsService, appInfo: appInfo) | |||
There was a problem hiding this comment.
The AppCoordinator should not be injected, other coordinators should not know is existence. It's adding an unnecessary object relation and avoid reusability.
| @@ -49,7 +49,7 @@ final class AppCoordinator: NSObject, AppCoordinatorType { | |||
| private weak var splitViewCoordinator: SplitViewCoordinatorType? | |||
| fileprivate weak var sideMenuCoordinator: SideMenuCoordinatorType? | |||
|
|
|||
| private let userSessionsService: UserSessionsService | |||
| let userSessionsService: UserSessionsService | |||
There was a problem hiding this comment.
We should try to keep it private and inject this instance from Coordinators to other Coordinators.
For places where we still can't inject instances such as UserSessionsService from side to side. The parent does not have the reference or the flow is started from ObjC class.
We can temporary define and use a shared instance like UserSessionsService.shared but we still continue to inject it from the outside like: CoordinatorParameters(userSessionsService: UserSessionsService.shared) to avoid having singleton inside Coordinators. And in AppCoordinator.init we can temporary use self.userSessionsService = UserSessionsService.shared until we can avoid singleton in the app.
- Update after code review
- Update after code review
- Fixed build issue on Jenkins
Fixes #4682
testable link: https://i.diawi.com/ybUnZi