Fix rendering link attachment preview with other attachment types #659
Conversation
SDK Size
|
| @@ -44,7 +44,8 @@ public struct MessageView<Factory: ViewFactory>: View { | |||
| } else if let poll = message.poll { | |||
| factory.makePollView(message: message, poll: poll, isFirst: isFirst) | |||
| } else if !message.attachmentCounts.isEmpty { | |||
| if messageTypeResolver.hasLinkAttachment(message: message) { | |||
| let hasOnlyLinks = message.attachmentCounts.keys.allSatisfy { $0 == .linkPreview } | |||
There was a problem hiding this comment.
As you stated in the PR description - probably good to move this in the resolver directly. It's a small breaking change in functionality, but it's actually a bug fix. We can deal with the inaccurate method name in a major release.
There was a problem hiding this comment.
The thing is that we are doing this kinda logic with video attachment as well, where we only render the video attachment view if there is not image attachment as well:
messageTypeResolver.hasVideoAttachment(message: message)
&& !messageTypeResolver.hasImageAttachment(message: message)
That is why opted for this approach as well 🤔
There was a problem hiding this comment.
I see. Maybe we can just put the check directly after the &&, so it's executed only when hasLinkAttachment is true?
…f-attachment-is-only-link
|
nuno-vieira
deleted the
fix/only-render-link-attachment-preview-if-attachment-is-only-link
branch
🔗 Issue Link
Fixes IOS-545
🎯 Goal
Fix rendering link attachment preview with other attachment types
🛠 Implementation
I've added a new condition, "hasOnlyLinks", to render the link attachment. This works, but I feel like the best solution would be for this to be the responsibility of
MessageTypeResolving. The problem is that the name of the method ishasLinkAttachment(), so if I added this boolean inside, the name loses its true meaning because we are checking instead "onlyHasLinkAttachment()`.So it feels like this
MessageTypeResolvingwould probably be better for the names to be something like:shouldRenderLinkAttachment(). This way, additional logic could be added to these methods, and it would be easier for customers to provide their own logic whether they want an attachment to be rendered or not. Maybe we can deprecate the old names, and introduce the "shouldRender" prefix instead, but probably out of scope for this PR.🧪 Testing
Add an image and a link in the same message. It should only render the image preview, not the link preview.
☑️ Checklist