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.

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

Location sharing: Integrate live location viewer screen with room screen #6114

Merged
merged 22 commits into from
May 4, 2022

Conversation

SBiOSoftWhare
Copy link
Contributor

@SBiOSoftWhare SBiOSoftWhare requested review from a team, pixlwave and gileluard and removed request for a team May 3, 2022 14:07
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

I've added some comments line line. One more global thought I had was as follows:

I struggled in places to understand names such as stopUserLiveLocationSharing, didUpdateUsersLiveLocation and usersLiveLocation as to whether these are about the current user's location or the location of lots of users. Would be great to have more documentation about these to help explain, or possibly using alternative names along the lines of currentUserLiveLocation/myLiveLocation for the current user and allLiveLocations/roomMemberLiveLocations for other users in the room.

Riot/Categories/MXSession.swift Outdated Show resolved Hide resolved
RiotSwiftUI/target.yml Outdated Show resolved Hide resolved

let errorHandler: (Error) -> Void = { error in

let viewController = self.roomViewController
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this local copy adds much to the implementation here?

Riot/Modules/Room/RoomCoordinator.swift Outdated Show resolved Hide resolved
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Thanks, the extra bit of documentation definitely helps 👍

@github-actions
Copy link

github-actions bot commented May 4, 2022

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/74m9k3

@SBiOSoftWhare SBiOSoftWhare merged commit ae6d2b5 into develop May 4, 2022
@SBiOSoftWhare SBiOSoftWhare deleted the steve/6081_lls_screen_integration branch May 4, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants