-
Notifications
You must be signed in to change notification settings - Fork 42
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
Feature/messaging #143
base: feature/messaging
Are you sure you want to change the base?
Feature/messaging #143
Conversation
Conflicts: TreeTracker.xcodeproj/project.pbxproj
…unread messages count.
… messages from planter.
… used in every viewController to change the backgroundColor.
…when finish fetching new messages.
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 for your great work Fred, clear you put a lot of time and effort into this!
Left some comments to look at 🙏
TreeTracker/UI/Views/Messaging/ChatList/ChatListViewModel.swift
Outdated
Show resolved
Hide resolved
TreeTracker/UI/Views/Messaging/ChatList/ChatListViewModel.swift
Outdated
Show resolved
Hide resolved
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.
Hey @frehelfer, I just landed on this PR after a while and while I was not able to build the app to see this first hand, I left some personal Nit changes. And from the attached video, the buttons look quite big
All in all this was massive work mate.🎉
@@ -99,3 +100,25 @@ | |||
"Settings.PhotoQuality.Option.Medium.Info" = "Medium resolution, medium data usage."; | |||
"Settings.PhotoQuality.Option.High.Title" = "High"; | |||
"Settings.PhotoQuality.Option.High.Info" = "High resolution, high data usage."; | |||
|
|||
//ChatList Screen | |||
"ChatList.SyncMessages.normal.Title" = "Sync Messages"; |
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.
Nit
"ChatList.SyncMessages.normal.Title" = "Sync Messages"; | |
"ChatList.SyncMessages.normal.Title" = "Update Messages"; |
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.
Hey @nshutinicolas, thank you for your review!
The only way to sync the messages, both fetch and send, is through this button, so I used the word "sync" to emphasize that the messages will be brought into alignment between the user's device and the remote server, I don't know if the word "update" has the same meaning.
As English is not my main language, I don't know which one would be more suitable for this situation. What do you think?
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.
As a dev the action is sync but as a user, they understand Refresh
or update
.
//ChatList Screen | ||
"ChatList.SyncMessages.normal.Title" = "Sync Messages"; | ||
"ChatList.SyncMessages.loading.Title" = "Loading..."; | ||
"ChatList.LastSyncLabel.WithouDate.Text" = "Please, tap the button above to sync messages."; |
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.
Nit
"ChatList.LastSyncLabel.WithouDate.Text" = "Please, tap the button above to sync messages."; | |
"ChatList.LastSyncLabel.WithouDate.Text" = "Please, tap the button above to update messages."; |
"ChatList.SyncMessages.normal.Title" = "Sync Messages"; | ||
"ChatList.SyncMessages.loading.Title" = "Loading..."; | ||
"ChatList.LastSyncLabel.WithouDate.Text" = "Please, tap the button above to sync messages."; | ||
"ChatList.LastSyncLabel.WithDate.Text" = "Last sync time:"; |
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.
"ChatList.LastSyncLabel.WithDate.Text" = "Last sync time:"; | |
"ChatList.LastSyncLabel.WithDate.Text" = "Last updated:"; |
|
||
//Messages Screen | ||
"Messages.Title" = "Admin"; | ||
"Messages.InputTextView.PlaceHolder" = "Click to write a message"; |
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.
I always felt like click is a web term🤷🏿♂️. Personal preference would be Start conversation
, send a message
or new message
} else if isHighlighted { | ||
backgroundColor = Asset.Colors.secondaryOrangeDark.color | ||
tintColor = .white | ||
} else { | ||
backgroundColor = Asset.Colors.secondaryOrangeLight.color | ||
tintColor = .white |
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.
Since isEnabled
and isHighlighted
are different, you can put them in separate conditional statements
something like
if !isEnabled {
...
}
if isHighlighted {
...
} else {
...
}
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! I'll make this change
// TODO: Change the button icon | ||
sendMessageButton.titleLabel?.font = FontFamily.Montserrat.bold.font(size: 30) | ||
sendMessageButton.backgroundColor = Asset.Colors.primaryGreen.color | ||
sendMessageButton.setTitle(">", for: .normal) |
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 use SFSymbols or asset instead of this
Sorry just seen the todo on top
# Conflicts: # TreeTracker.xcodeproj/project.pbxproj
Overview of changes
Created ChatListScreen
Created MessageScreen
Created AnnounceScreen
Created Survey Screen
Screenshots (if applicable)