Fix/1239 message wrapper for render library #1297

AudreyKj · 2021-03-19T08:14:04Z

changes

updated typing related to the templates api (both in frontend and httpClient)
created SharedServices to share fallbackImage across frontend and the render library
typing for rendering content
render library revamp with better typing + cleaned its components from all unnecessary props
moved all message rendering logic to InfoMessageWrapper and updated all the frontend

screenshots
--> as all the message-rendering logic is moved to the wrapper, the template modal renders nicely all types of templates + the fallback image improves user experience for all non-valid images

…ponents

…nder library creating sharedServices

…change

AudreyKj · 2021-03-19T08:42:02Z

lib/typescript/render/shared.ts

  return {
    fromContact,
-    contact: props.contact ?? null,
-    commandCallback: props.commandCallback,
-    sentAt: props.lastInGroup ? formatTime(props.message.sentAt) : null,
  };


All the logic exclusive to rendering messages is moved to the wrapper <InfoMessageWrapper />, which displays the Avatar and sentAt time along with the message's content.

This wrapper is also responsible for determining is which side the messages are displayed. The components inside the render library are stripped down from this logic and are just pure components.

RenderProps contains the base props common to all the content that the library needs to render (i.e. we need the source and the renderedContent for both messages and templates). It's the base type from MessageRenderProps and TemplateRenderProps . <InfoMessageWrapper /> takes in MessageRenderProps to get the necessary data for the message rendering logic.

DefaultRenderingProps are needed by some components that the library needs to render, for all content (for all the components that use <Text />, as this props will determine the backgroundColor of the message)

AudreyKj · 2021-03-19T08:57:40Z

lib/typescript/httpclient/model/Content.ts

+
+export function isFromContact(message: RenderedContentUnion) {
+  if (message && 'senderType' in message) return message?.senderType === SenderType.sourceContact;
+}


typing for render library: RenderedContentUnion represents the type for the content that the library is rendering

AudreyKj · 2021-03-19T09:00:09Z

frontend/ui/src/reducers/data/templates/index.ts

@@ -22,7 +23,8 @@ export default function templatesReducer(state = defaultState, action: Action):
    case getType(actions.listTemplatesAction):
      return {
        ...state,
-        all: action.payload,
+        all: action.payload.templates,
+        source: action.payload.source,
      };


This keeps track of the source of the templates that are listed. This is needed because we fetch the list of templates only when the conversation's source channel changes (as the api lists templates based on the source).

AudreyKj · 2021-03-19T09:08:11Z

frontend/sharedServices/fallbackImage.ts

+  }
+
+  event.currentTarget.alt = 'fallback image';
+};


With @thorstenairy we had the idea of creating SharedServices that contains helper functions needed by both the frontend and our libraries. Here fallbackImage serves for images in both the frontend and the render library.

yeah so this was brought up many times and I should probably write down the answer eheh

the tl;dr is "shared" "util" is always bad as then we mix things that have nothing to do with each other. (I think last I talked to @thorstenairy exactly about this because he introduced a shared lib with was a date lib)

A better approach is to have small libraries that solve a specific problem.

to me, it feels like here we're introducting an avatar library. So best is 1) call it what it does 2) that way we can move more "avatar related things" there 3) by far themost important point: we do not open the door to catch all libraries

@lucapette the idea was to also use this function for the fallback images in the render library.

I totally get the point of not falling in 'catch all libraries' so should we create 2 libraries: one avatar library used for the channel avatars (and possibly also the Avatar component), and another library (fallback image library) for the fallback images in the render library? (but then we would duplicate code)

duplicating a little of code is better than the wrong abstraction for sure

ok we talked offline about it: we do not need to introduce libraries as the abstraction is not worth it. //c @thorstenairy

AudreyKj · 2021-03-19T09:09:23Z

lib/typescript/httpclient/model/Message.ts

-  chatplugin = 'chatplugin',
-  smsTwilio = 'twilio.sms',
-  whatsappTwilio = 'twilio.whatsapp',
-}


the enum SourceType was duplicated in both Message and Channels in the httpClient library.

source was a better name :D (sorry... I suggested source type somewhere else.... without realising we call the field source everywhere in the api)

AudreyKj · 2021-03-19T09:11:33Z

lib/typescript/render/components/RichCard/Media/index.tsx

@@ -18,5 +19,6 @@ export const Media = ({height, contentInfo: {altText, fileUrl}}: MediaRenderProp
    className={`${styles.mediaImage} ${
      height === MediaHeight.tall ? styles.tall : height === MediaHeight.medium ? styles.medium : styles.short
    }`}
+    onError={(event: React.SyntheticEvent<HTMLImageElement, Event>) => fallbackImage(event, 'mediaImage')}
  />


added fallbackImage for all the images inside the render library. This fallback does not work for video, I've created an issue for this #1299

chrismatix

First pass. Amazing work already!

chrismatix · 2021-03-19T08:50:05Z

frontend/ui/src/pages/Inbox/MessageInput/index.tsx

@@ -47,6 +47,9 @@ const MessageInput = (props: MessageInputProps & ConnectedProps<typeof connector
  const sendButtonRef = useRef(null);
  const emojiDiv = useRef<HTMLDivElement>(null);

+  const conversationIdParams = useParams();
+  const currentConversationId: string = conversationIdParams[Object.keys(conversationIdParams)[0]];


If possible I would suggest to always use the getCurrentConversation selector since it also uses the routing params but in a declarative and typed way :)

chrismatix · 2021-03-19T08:53:36Z

lib/typescript/render/shared.ts

-  message: RenderedContent;
+interface RenderProps {
+  contentType: 'message' | 'template';
+  renderedContent: RenderedContentUnion;


nitpick: At this point the content is not yet rendered so maybe just content would be enough (applies to other places too)

chrismatix · 2021-03-19T09:06:08Z

lib/typescript/render/shared.ts

-import {isFromContact, RenderedContent, Contact} from 'httpclient';
-import {formatTime} from 'dates';
-import {DefaultMessageRenderingProps} from './components';
+import {isFromContact, RenderedContentUnion} from 'httpclient';


nitpick: shared.ts was never a great name for this file. Looking at it, what do you think about props.ts or something else?

yeah I think props.ts makes sense :)

chrismatix · 2021-03-19T09:20:13Z

frontend/sharedServices/BUILD

+load("@com_github_airyhq_bazel_tools//lint:buildifier.bzl", "check_pkg")
+load("@com_github_airyhq_bazel_tools//web:typescript.bzl", "ts_web_library")
+
+package(default_visibility = ["//visibility:public"])


I get the intention of this module, but I think the name is too generic and not quite accurate (It hosts a component and not a service). Only thing I can think of for now would be frontend/components

frontend/lib/avatar would be my suggestion (given the previous point I made about it)

…elector

AudreyKj · 2021-03-19T16:25:54Z

frontend/ui/src/pages/Channels/ConnectedChannelsBySourceCard/index.tsx

-import {Channel, SourceType} from 'httpclient';
-import {SourceTypeInfo} from '../MainPage';
+import {Channel, Source} from 'httpclient';
+import {SourceInfo} from '../MainPage';
 import {fallbackImage} from '../../../services/image/index';


here updated naming to follow api naming Source

bitboxer

Awesome stuff 🥳 . Some minor comments. After the rebase is done I will 🟢 this.

bitboxer · 2021-03-23T08:56:23Z

frontend/ui/src/components/AvatarImage/index.tsx

@@ -14,7 +14,7 @@ const AvatarImage = (props: AvatarProps) => {

  return (
    <div className={styles.avatar}>
-      <img className={styles.avatarImage} src={contact.avatarUrl || fallbackAvatar} />
+      <img className={styles.avatarImage} src={(contact && contact.avatarUrl) || fallbackAvatar} />


You could use the new optional chaining operator instead:

Suggested change

<img className={styles.avatarImage} src={(contact && contact.avatarUrl) || fallbackAvatar} />

<img className={styles.avatarImage} src={(contact?.avatarUrl) || fallbackAvatar} />

bitboxer · 2021-03-23T09:06:27Z

frontend/ui/src/pages/Inbox/Messenger/ConversationMetadata/index.tsx

@@ -184,7 +184,7 @@ const ConversationMetadata = (props: ConnectedProps<typeof connector>) => {
              <AvatarImage contact={contact} />
            </div>

-            <div className={styles.displayName}>{contact.displayName}</div>
+            <div className={styles.displayName}>{contact && contact.displayName}</div>


Like above:

Suggested change

<div className={styles.displayName}>{contact && contact.displayName}</div>

<div className={styles.displayName}>{contact?.displayName}</div>

bitboxer · 2021-03-23T09:08:31Z

infrastructure/.vagrant/machines/airy-core/virtualbox/action_provision

@@ -0,0 +1 @@
+1.5:b2a9215c-db31-4f1b-b3db-1e3a1e879bbe


I think the .vagrant folder is gone now because minikube ?

yes! don't know how this snugged into my PR!

lucapette

Looking great!

I think we should merge, I'm requesting changes so we don't forget to remove all the vagrant files before we merge :)

bitboxer

AudreyKj added 9 commits March 16, 2021 18:30

fixed typing for render library, fixed infomessage wrapper, fixed com…

5acd264

…ponents

fixed typing render library, rebased, and added fallback image for re…

3f539c2

…nder library creating sharedServices

fixed fallback image and edited the chat plugin

1a11644

Merge branch 'develop' into fix/1239-message-wrapper-for-render-library

1fffe88

refined typing, edited providers, updated template endpoint with api …

3ccba0b

…change

further refined typing, improved template list, fix message list

ccf4769

fixed templates reducer

b3b3b48

rebased and fixed google render

8293ab0

fixed typing and edited components

809db6a

github-actions bot added the fix label Mar 19, 2021

AudreyKj commented Mar 19, 2021

View reviewed changes

AudreyKj requested review from chrismatix, AitorAlgorta, lucapette, ogbeche77 and stayprodio March 19, 2021 09:14

AudreyKj marked this pull request as ready for review March 19, 2021 09:14

chrismatix reviewed Mar 19, 2021

View reviewed changes

AudreyKj mentioned this pull request Mar 19, 2021

Remove additional sentAt string from Facebook button template #1305

Closed

AudreyKj added 4 commits March 19, 2021 14:40

improvments to pr: fixed typing, imagefallback, conversation id via s…

d5ee75f

…elector

rebased

ccd7ad8

fixing small import errors

fb9df74

fixed naming sourcetype

1452163

AudreyKj commented Mar 19, 2021

View reviewed changes

Merge branch 'develop' into fix/1239-message-wrapper-for-render-library

3235ecf

bitboxer reviewed Mar 23, 2021

View reviewed changes

lucapette suggested changes Mar 23, 2021

View reviewed changes

resolved conflicts

a249537

bitboxer previously approved these changes Mar 23, 2021

View reviewed changes

untracked file

3dfc2be

AudreyKj dismissed bitboxer’s stale review via 3dfc2be March 23, 2021 12:50

added back file

da30a07

lucapette approved these changes Mar 23, 2021

View reviewed changes

AudreyKj merged commit c389630 into develop Mar 24, 2021

AudreyKj deleted the fix/1239-message-wrapper-for-render-library branch March 24, 2021 09:24

ljupcovangelski added a commit that referenced this pull request Apr 25, 2022

[#1297] Update filter in release scripts

949e22f

ljupcovangelski added a commit that referenced this pull request Apr 25, 2022

[#1297] Update filter in release scripts

d7a8c07

ljupcovangelski added a commit that referenced this pull request Apr 25, 2022

[#1297] Update filter in release scripts (#3070)

bdb5010

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/1239 message wrapper for render library #1297

Fix/1239 message wrapper for render library #1297

AudreyKj commented Mar 19, 2021 •

edited

AudreyKj Mar 19, 2021 •

edited

AudreyKj Mar 19, 2021

AudreyKj Mar 19, 2021 •

edited

AudreyKj Mar 19, 2021

lucapette Mar 19, 2021

AudreyKj Mar 19, 2021 •

edited

lucapette Mar 19, 2021

lucapette Mar 19, 2021

AudreyKj Mar 19, 2021 •

edited

lucapette Mar 19, 2021

AudreyKj Mar 19, 2021

chrismatix left a comment

chrismatix Mar 19, 2021

chrismatix Mar 19, 2021

chrismatix Mar 19, 2021

AudreyKj Mar 19, 2021

chrismatix Mar 19, 2021

lucapette Mar 19, 2021

AudreyKj Mar 19, 2021

bitboxer left a comment

bitboxer Mar 23, 2021

bitboxer Mar 23, 2021

bitboxer Mar 23, 2021

AudreyKj Mar 23, 2021

lucapette left a comment

bitboxer left a comment

	<img className={styles.avatarImage} src={(contact && contact.avatarUrl) \|\| fallbackAvatar} />
	<img className={styles.avatarImage} src={(contact?.avatarUrl) \|\| fallbackAvatar} />

	<div className={styles.displayName}>{contact && contact.displayName}</div>
	<div className={styles.displayName}>{contact?.displayName}</div>

		@@ -0,0 +1 @@
		1.5:b2a9215c-db31-4f1b-b3db-1e3a1e879bbe

Fix/1239 message wrapper for render library #1297

Fix/1239 message wrapper for render library #1297

Conversation

AudreyKj commented Mar 19, 2021 • edited

AudreyKj Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudreyKj Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudreyKj Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudreyKj Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismatix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitboxer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucapette left a comment

Choose a reason for hiding this comment

bitboxer left a comment

Choose a reason for hiding this comment

AudreyKj commented Mar 19, 2021 •

edited

AudreyKj Mar 19, 2021 •

edited

AudreyKj Mar 19, 2021 •

edited

AudreyKj Mar 19, 2021 •

edited

AudreyKj Mar 19, 2021 •

edited