-
Notifications
You must be signed in to change notification settings - Fork 439
[NEW] Support autocompletion in new composer #2224
[NEW] Support autocompletion in new composer #2224
Conversation
….com/RocketChat/Rocket.Chat.iOS into feat/autocomplete_new_composer
Codecov Report
@@ Coverage Diff @@
## feature/integrate-rc-view-controller #2224 +/- ##
========================================================================
+ Coverage 27.83% 28.11% +0.27%
========================================================================
Files 402 405 +3
Lines 15030 15186 +156
========================================================================
+ Hits 4184 4269 +85
- Misses 10846 10917 +71
Continue to review full report at Codecov.
|
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 one @cardoso! The tests are failing for this changes (I believe is because of the refactor you did on the AvatarView). Also, added one suggestion about splitting the code in another view model just for the hints... let me know what you think please! 👍
emojis.forEach { | ||
viewModel.hints.append($0.suggestion) | ||
} | ||
} |
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.
What do you think about moving this method to the view model as well? Maybe we can have one view model just for the hints? 🤔
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 saw that you guys are doing this pattern, so I did the same. I created a MessageComposerViewModel and put this logic there 👍
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.
👍
} | ||
|
||
// MARK: Themeable | ||
|
||
extension AvatarView { | ||
override func applyTheme() { | ||
labelInitials?.textColor = .white | ||
labelInitials.textColor = .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.
👌
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 believe you need to call super here... don't you?
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.
Not needed in this particular case I think, but I can do it for consistency.
@RocketChat/ios
I had to refactor AvatarView to be programmatic so it can work with UserHintCell