-
Notifications
You must be signed in to change notification settings - Fork 500
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] Live location cell in timeline #6055
Conversation
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/uQNY5V |
private lazy var incomingTimerFormatter: DateFormatter = { | ||
let dateFormatter = DateFormatter() | ||
dateFormatter.dateFormat = "HH:mm" | ||
return dateFormatter | ||
}() | ||
|
||
private lazy var outgoingTimerFormatter: DateComponentsFormatter = { | ||
let formatter = DateComponentsFormatter() | ||
formatter.zeroFormattingBehavior = .dropAll | ||
formatter.allowedUnits = [.hour, .minute, .second] | ||
formatter.unitsStyle = .brief | ||
return formatter | ||
}() |
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.
We usually put properties at the top of the file
@@ -76,7 +108,8 @@ class RoomTimelineLocationView: UIView, NibLoadable, Themable, MGLMapViewDelegat | |||
|
|||
public func displayLocation(_ location: CLLocationCoordinate2D, |
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.
You can set this function private and use to separate methods. We should also handle all states of current user live location sharing. For example:
struct RoomTimelineLocationViewData {
let location: CLLocationCoordinate2D
let userAvatarData: AvatarViewData?
let mapStyleURL: URL
}
struct LiveLocationBannerViewData {
let icon: UIImage
let title: String
let timeLeftString: String?
let rightButtonTitle: String?
var showRightButton: Bool {
return rightButtonTitle != nil
}
}
enum TimelineLiveLocationViewState {
case outgoing(_ status: LiveLocationSharingStatus) // live location started from current user
case incoming(_ expiringDate: Date) // live location started by other users
}
enum LiveLocationSharingStatus {
case starting
case started(_ timeleft: TimeInterval)
case failure
case stopped
}
public func displayStaticLocation(with viewData: RoomTimelineLocationViewData) {
}
public func displayLiveLocation(with viewData: RoomTimelineLocationViewData, liveLocationViewState: TimelineLiveLocationViewState) {
let bannerViewData = self.liveLocationBannerViewData(from viewState: TimelineLiveLocationViewState)
...
}
let timerString: String? | ||
if isIncomingLocation { | ||
timerString = VectorL10n.locationSharingLiveTimerIncoming(incomingTimerFormatter.string(from: Date(timeIntervalSince1970: timestamp))) | ||
} else if let outgoingTimer = outgoingTimerFormatter.string(from: Date(timeIntervalSince1970: timestamp).timeIntervalSinceNow) { |
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.
The timestamp is in milliseconds
func didTapStopButton() | ||
func didTapRetryButton() |
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.
In general we are referencing the delegate instance:
func roomTimelineLocationViewDidTapStopButton(_ roomTimelineLocationView: RoomTimelineLocationView)
func roomTimelineLocationViewDidTapRetryButton(_ roomTimelineLocationView: RoomTimelineLocationView)
Note: You can also use closures if want instead of delegation
@@ -105,6 +105,14 @@ class DefaultTheme: NSObject, Theme { | |||
|
|||
var roomCellOutgoingBubbleBackgroundColor: UIColor = UIColor(rgb: 0xE7F8F3) | |||
|
|||
var roomCellLocalisationTextColor: UIColor = UIColor(rgb: 0x17191C) |
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.
This color seems redundant with primaryContent
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.
The problem is, "primaryContent" color is White on dark theme, so it didn't work on the white background of the cell.
@@ -105,6 +105,14 @@ class DefaultTheme: NSObject, Theme { | |||
|
|||
var roomCellOutgoingBubbleBackgroundColor: UIColor = UIColor(rgb: 0xE7F8F3) | |||
|
|||
var roomCellLocalisationTextColor: UIColor = UIColor(rgb: 0x17191C) | |||
|
|||
var roomCellLocalisationStartedColor: UIColor = UIColor(rgb: 0x5C56F5) |
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.
We can maybe precise icon: roomCellLocalisationIconStartedColor
or use a generic liveLocationSharingIconStartedColor
@@ -155,6 +155,8 @@ class RoomTimelineLocationView: UIView, NibLoadable, Themable, MGLMapViewDelegat | |||
clipsToBounds = true | |||
layer.borderWidth = Constants.cellBorderRadius | |||
layer.cornerRadius = Constants.cellCornerRadius | |||
|
|||
theme = ThemeService.shared().theme |
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.
it should be updated in update(theme: Theme)
also
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.
Great!
Maybe it's a visual illusion because of the shape of the letter but weirdly, it seems that the Avatar pointer isn't quite neat with the spacing of the border.
See:
Also, the pointer on the loading/Stop state image in the timeline seems very big. Perhaps we could just load the map image and then the pointer separately?
I have copies of the background map in the component section of Figma here: https://www.figma.com/file/uthEK6xuo9hSQRdWvPAre5/?node-id=470%3A51660
You can export in whatever format is best for you
We also need to change the style of the map and button on Dark mode. I'll add this in my to do list and will point you to a reference when ready.
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.
Thanks all good I'll just need to review the dark theme for next PR
close #6029
For now, the live location event is not handle, we will have to come back to this part when it will be done.