feat: chat bubble wrapper for text messages - WPB-19273

[WilhelmOks]

WPB-19273 [iOS] Conversation Message Content embedding into Simple Chat Bubbles

Issue

Text Message cells are wrapped into bubbles.

The color of the bubble depends on if it's my own message or from someone else and also depends on the color selected by the user as their accent color.

The corner radius of all cell types (except small icons like reactions) like reply and file has been adjusted to be the same as the new chat bubble corner radius.

The correct alignment will be implemented in WPB-19271 and is not in scope of this PR.

Testing

If the chatBubblesSimple DeveloperFlag is disabled, everything should look and behave exactly as it was before.

If the chatBubblesSimple DeveloperFlag is enabled:

• text messages should be inside of a chat bubble
  • the color of the chat bubble from other users should be gray
  • the color of the chat bubble from the self user should be their selected accent color
• dark and light theme should look like in the design
• the corner radius of all content cells like image and reply should be the same radius as the chat bubble

[github-actions]

Test Results

1 991 tests   1 963 ✅  3m 8s ⏱️
  337 suites     28 💤
    2 files        0 ❌

Results for commit 1ab2a3e.

♻️ This comment has been updated with latest results.

[dmitrysimkin]

Looks good, nice work 👍
Before approving, can you also please some snapshot tests @WilhelmOks ?

[dmitrysimkin]

suggestion: Since it's same in several files I'd suggest to introduce a constant and reuse if DeveloperFlag.chatBubblesSimple.isOn ...

[WilhelmOks]

Not sure what you mean by reuse the if condition.

Do you mean extracting it into a property like this?

var: cornerRadius: CGFloat {
        let cornerRadius: CGFloat = if DeveloperFlag.chatBubblesSimple.isOn {
            ConversationMessageContainerView.bubbleCornerRadius
        } else {
            12
        }
}

but then it would still be repeated in multiple files.

[dmitrysimkin]

I mean more like this

struct ChatBubblesConstants {
    var cornerRadius: CGFloat {
        if DeveloperFlag.chatBubblesSimple.isOn {
            return ConversationMessageContainerView.bubbleCornerRadius
        } else {
            return 12
        }
    }
}

And then on caller side: containerView.layer.cornerRadius = ChatBubblesConstants.cornerRadius
Do you think there would be a benefit?

[WilhelmOks]

I see some issues with that:

• the old value for the corner radius isn't always 12. For some views like the reply cell it's smaller.
• ChatBubblesConstants for me suggests that it's about the new chat bubbles. So putting the old value there (12) feels wrong.
• special treatment for the corner radius while the other numeric values remain number literals.

[WilhelmOks]

Looks good, nice work 👍 Before approving, can you also please some snapshot tests @WilhelmOks ?

Assuming this is a test which compares the visual output to an image from the design:
I think our designs for the simple solution are not nearly exact enough to make a pixel perfect comparison.

[dmitrysimkin]

Looks good, nice work 👍 Before approving, can you also please some snapshot tests @WilhelmOks ?

Assuming this is a test which compares the visual output to an image from the design: I think our designs for the simple solution are not nearly exact enough to make a pixel perfect comparison.

What do you mean by it's a test? As I understand it supposed to be released, right? Tests help make sure that while working on other parts of application already existed functionality behaves in same way. Not critical for me, we just usually add snapshot tests to views we add. cc @johnxnguyen

[WilhelmOks]

Looks good, nice work 👍 Before approving, can you also please some snapshot tests @WilhelmOks ?

Assuming this is a test which compares the visual output to an image from the design: I think our designs for the simple solution are not nearly exact enough to make a pixel perfect comparison.

What do you mean by it's a test? As I understand it supposed to be released, right? Tests help make sure that while working on other parts of application already existed functionality behaves in same way. Not critical for me, we just usually add snapshot tests to views we add. cc @johnxnguyen

Hm, probably I misunderstood what is a snapshot test.
Can you provide to me a quick explanation or a guide how such a snapshot test would be implemented in the Wire project?

[johnxnguyen]

Hm, probably I misunderstood what is a snapshot test. Can you provide to me a quick explanation or a guide how such a snapshot test would be implemented in the Wire project?

@WilhelmOks snapshot tests kind of what you think, but the implemented views aren't compared to the designed view, rather you implement the view according to the design, snapshot it (record a reference image from your view), then future runs of the test will assert that the view still matches the reference.

Take a look at ConversationTextMessageTests.swift, basically you create your view, run the verify method, it if it doesn't find a reference already it will record one, otherwise it will compare the view with the reference.

[johnxnguyen]

Looks good! Left a suggestion/question.

[johnxnguyen]

suggestion: this seems to be more appropriate placed inside the func configure(with object: Configuration, animated: Bool) method, but I then you'd need to add an extra field to the configuration for the bubble style. What do you think?

[WilhelmOks]

My first attempt was to set both values, isBubble and bubbleStyle, in configure(with object: Configuration, animated: Bool) but the problem is that for bubbleStyle I need the info if this is the self user message. And message is nil at this point.
So I moved the code to didSet of the message property, because there I can be certain that the message has been set and is not nil.

[dmitrysimkin]

might be removed if not used anymore

[WilhelmOks]

The record constant?
I feel like it's useful for later when we change the bubble layout and need to generate new snapshots for all tests again in one go.
But I have no strong opinion on that.