-
Notifications
You must be signed in to change notification settings - Fork 111
Make sendMessage in MessageComposerViewModel open #779
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
Conversation
SDK Size
|
@Binding var editedMessage: ChatMessage? | ||
|
||
private let recordingViewHeight: CGFloat = 80 | ||
private let messageSendingParameters: MessageSendingParameters |
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 I see with this approach is that these parameters will be static, right? The customer set's it once, but then it can't change it dynamically. So if you set skipPush: true
, then all messages will have skipPush to true
right?
Other alternatives I'm thinking:
- Making the
ViewModel.sendMessage()
open (This way the customer can override the function and change the parameters before calling thesuper.sendMessage()
- Make
messageSendingParameters
avar
and notlet
, and/or maybe provide a closure to change the paremeters before calling the sendMessage() 🤔
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 first one looks like it should be more than enough, though. Unless I'm missing something.
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.
subclassing won't help - customers don't want to customize the send message method, but pass different params from the view layer. Even if we make it open (which we can for sure), they would still need to re-implement the composer view.
A different option I considered is maybe expose the whole closure and let customers handle it all by themselves, basically this part can be nil
by default with this default implementation:
viewModel.sendMessage(
quotedMessage: quotedMessage,
editedMessage: editedMessage,
isSilent: messageSendingParameters.isSilent,
skipPush: messageSendingParameters.skipPush,
skipEnrichUrl: messageSendingParameters.skipEnrichUrl,
extraData: messageSendingParameters.extraData
) {
quotedMessage = nil
editedMessage = nil
onMessageSent()
}
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.
By subclassing, they can change the paremeters without changing the existing implementation. Example:
open class CustomComposerViewModel: MessageComposerViewModel {
public var customShouldSkip: Bool? = false
open override func sendMessage(
quotedMessage: ChatMessage?,
editedMessage: ChatMessage?,
isSilent: Bool = false,
skipPush: Bool = false,
skipEnrichUrl: Bool = false,
extraData: [String : RawJSON] = [:],
completion: @escaping () -> Void
) {
super.sendMessage(
quotedMessage: quotedMessage,
editedMessage: editedMessage,
isSilent: isSilent,
skipPush: customShouldSkip ?? skipPush,
skipEnrichUrl: skipEnrichUrl,
extraData: extraData,
completion: completion
)
}
}
Why is this not enough?
Inside the view model, the customer also has all the data and context needed to change the parameters 🤔
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.
ah yes, you are totally right! They can pass the custom VM instead. Let me change stuff.
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.
done
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.
LGTM! ✅
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.
✅
|
🔗 Issue Link
Resolves https://linear.app/stream/issue/IOS-732/provide-message-sending-params-to-messagecomposerview.
🎯 Goal
Describe why we are making this change.
🛠 Implementation
Provide a description of the implementation.
🧪 Testing
Describe the steps how this change can be tested (or why it can't be tested).
🎨 Changes
Add relevant screenshots or videos showcasing the changes.
☑️ Checklist