Skip to content
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

[#1808] Improved lastMessageSent #1821

Merged
merged 3 commits into from
May 28, 2021

Conversation

stayprodio
Copy link
Contributor

closes #1808

@stayprodio stayprodio requested a review from AudreyKj May 21, 2021 10:35
@github-actions github-actions bot added the fix label May 21, 2021
const lastMessageContent = conversation.lastMessage.content;

if (typeof lastMessageContent === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code should be in the rendering library, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is in the rendering lib, we just didn't use it. We should refactor the conversationListItem so it uses the render lib for every source, don't we @AitorAlgorta @chrismatix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to use the render lib also fro the preview of the message? I am not sure what is best

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use the render lib to show the preview of the message, that way it shows more of a description than just an icon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AitorAlgorta Picking up on your other comment: Doing it in the render library is simpler, because each renderer is split into a mapper and a renderer. The SourceMessagePreview can re-use the mapper part. This is reason why I did the split in the first place :)

Furthermore, we can simplify the preview into "text" vs "something else" (images, data, audio, etc.) to make the components simpler. So we add two components and then a SourceMessagePreview, where every source uses its mapper and then maps text to the text component and anything else to the "something else" (data) component.

AudreyKj
AudreyKj previously approved these changes May 21, 2021
Copy link
Contributor

@AudreyKj AudreyKj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

source={conversation.channel.source}
contentType="message"
content={conversation.lastMessage}
isTwilioConversationListItem={true}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API of the render library needs to be source agnostic. What does this attribute change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the styling of the message itself, so it not a bubble as in messageList @chrismatix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @chrismatix the render lib should be agnostic. That's what I meant in my previous comment. Either we don't use the render lib for this and we do it outside checking all the types, or have a different component in the render lib for the preview of the message. But we shouldn't change the SourceMessage for this. @airyhq/codeowners-frontend

@stayprodio stayprodio merged commit d251576 into develop May 28, 2021
@stayprodio stayprodio deleted the fix/1808-twilioSMS-lastMessageSent branch May 28, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong lastMessageIcon for Twilio SMS messages
5 participants