[Feature] Add the makeAttachmentTextView method to ViewFactory
#1013
Conversation
martinmitrevski
left a comment
martinmitrevski
left a comment
There was a problem hiding this comment.
Looks good, left few small comments.
| let injectedBackgroundColor: UIColor? | ||
|
|
||
| public init(message: ChatMessage, injectedBackgroundColor: UIColor? = nil) { | ||
| public init(factory: Factory, message: ChatMessage, injectedBackgroundColor: UIColor? = nil) { |
There was a problem hiding this comment.
We should put a default parameter DefaultViewFactory.shared to avoid breaking changes here.
There was a problem hiding this comment.
we should put the default param here
| } | ||
|
|
||
| public func makeAttachmentTextView( | ||
| message: ChatMessage |
There was a problem hiding this comment.
We now follow an approach where we put an options class instead of the params directly. So this would be something like AttachmentTextViewOptions, with one property message (to enable adding new properties in the future).
There was a problem hiding this comment.
The access level of AttachmentTextViewOptions should be open, right? To allow developers to subclass and extend the class further in the feature.
There was a problem hiding this comment.
no, just public is enough - we would like to control the extensibility of these classes internally.
| /// - Parameter message: The message containing the attachment. | ||
| /// - Returns: The view shown in the attachment text slot. | ||
| func makeAttachmentTextView( | ||
| message: ChatMessage |
There was a problem hiding this comment.
We usually add a unit test to make sure that we don't change the returned type (see ViewFactory_Tests).
|
Thanks @martinmitrevski for the review. Pls take another look at new updates |
| @Binding public var scrolledId: String? | ||
|
|
||
| public init(factory: Factory, message: ChatMessage, contentWidth: CGFloat, isFirst: Bool, scrolledId: Binding<String?>) { | ||
| public init(factory: Factory = DefaultViewFactory.shared, message: ChatMessage, contentWidth: CGFloat, isFirst: Bool, scrolledId: Binding<String?>) { |
There was a problem hiding this comment.
this one is actually fine, we shouldn't change its signature, check the other one AttachmentTextView
| let injectedBackgroundColor: UIColor? | ||
|
|
||
| public init(message: ChatMessage, injectedBackgroundColor: UIColor? = nil) { | ||
| public init(factory: Factory, message: ChatMessage, injectedBackgroundColor: UIColor? = nil) { |
There was a problem hiding this comment.
we should put the default param here
| } | ||
|
|
||
| // Options for the attachment text view. | ||
| open class AttachmentTextViewOptions { |
There was a problem hiding this comment.
this should be public
There was a problem hiding this comment.
Sure. Pls take another look. Thank you
martinmitrevski
left a comment
martinmitrevski
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Looks good from my side ✅ @testableapple can you please run the checks before merging?
testableapple
changed the base branch from
develop
to
test/make-attachment-text-view
testableapple
merged commit faa2765
into
GetStream:test/make-attachment-text-view
3 participants
🔗 Issue Links
N/A
🎯 Goal
Be able to customize the text view for messages that contain both attachments and text.
📝 Summary
Add a method to the ViewFactory that makes the attachment text view
🛠 Implementation
Add the
makeAttachmentTextViewmethod to ViewFactory to enable customization🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
🧪 Manual Testing Notes
Explain how this change can be tested manually, if applicable.
☑️ Contributor Checklist
docs-contentrepo