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

[IMPROVE] Message rewrite code structure #24723

Merged
merged 3 commits into from
Mar 7, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions client/lib/isE2EEMessage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { IMessage } from '../../definition/IMessage';

export const isE2EEMessage = (message: IMessage): boolean => message.t === 'e2e';
111 changes: 31 additions & 80 deletions client/views/room/MessageList/components/Message.tsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,14 @@
/* eslint-disable complexity */
import { css } from '@rocket.chat/css-in-js';
import {
Box,
Message as MessageTemplate,
MessageBody,
MessageContainer,
MessageHeader,
MessageLeftContainer,
MessageName,
MessageRole,
MessageRoles,
MessageTimestamp,
MessageUsername,
MessageReactions,
MessageStatusPrivateIndicator,
MessageReactionAction,
Icon,
} from '@rocket.chat/fuselage';
import React, { FC, memo } from 'react';

Expand All @@ -28,32 +21,22 @@ import Broadcast from '../../../../components/Message/Metrics/Broadcast';
import Discussion from '../../../../components/Message/Metrics/Discussion';
import Thread from '../../../../components/Message/Metrics/Thread';
import UserAvatar from '../../../../components/avatar/UserAvatar';
import { useSetting } from '../../../../contexts/SettingsContext';
import { TranslationKey, useTranslation } from '../../../../contexts/TranslationContext';
import { useUserId } from '../../../../contexts/UserContext';
import { useUserData } from '../../../../hooks/useUserData';
import { getUserDisplayName } from '../../../../lib/getUserDisplayName';
import { isE2EEMessage } from '../../../../lib/isE2EEMessage';
import { UserPresence } from '../../../../lib/presence';
import MessageBlock from '../../../blocks/MessageBlock';
import MessageLocation from '../../../location/MessageLocation';
import {
useMessageActions,
useMessageOembedIsEnabled,
useMessageOembedMaxWidth,
useMessageRunActionLink,
} from '../../contexts/MessageContext';
import {
useMessageListShowRoles,
useMessageListShowUsername,
useMessageListShowRealName,
useOpenEmojiPicker,
useReactionsFilter,
useReactToMessage,
useUserHasReacted,
} from '../contexts/MessageListContext';
import { useMessageActions, useMessageRunActionLink } from '../../contexts/MessageContext';
import { useMessageListShowUsername, useMessageListShowRealName } from '../contexts/MessageListContext';
import { MessageIndicators } from './MessageIndicators';
import { MessageReaction } from './MessageReaction';
import ReactionsList from './MessageReactionsList';
import ReadReceipt from './MessageReadReceipt';
import RolesList from './MessageRolesList';
import Toolbox from './Toolbox';
import OEmbedList from './UrlPreview';
import PreviewList from './UrlPreview';

const Message: FC<{ message: IMessage; sequential: boolean; subscription?: ISubscription }> = ({
message,
Expand All @@ -70,23 +53,15 @@ const Message: FC<{ message: IMessage; sequential: boolean; subscription?: ISubs

const runActionLink = useMessageRunActionLink();

const hasReacted = useUserHasReacted(message);
const reactToMessage = useReactToMessage(message);
const filterReactions = useReactionsFilter(message);

const oembedIsEnabled = useMessageOembedIsEnabled();
const oembedWidth = useMessageOembedMaxWidth();

const openEmojiPicker = useOpenEmojiPicker(message);

const isReadReceiptEnabled = useSetting('Message_Read_Receipt_Enabled');

const showRoles = useMessageListShowRoles();
const showRealName = useMessageListShowRealName();
const user: UserPresence = { ...message.u, roles: [], ...useUserData(message.u._id) };
const usernameAndRealNameAreSame = !user.name || user.username === user.name;
const showUsername = useMessageListShowUsername() && showRealName && !usernameAndRealNameAreSame;

const isEncryptedMessage = isE2EEMessage(message);
const encryptedMessageIsPending = isEncryptedMessage && message.e2e === 'pending';
const messageIsReady = !isEncryptedMessage || !encryptedMessageIsPending;
Copy link
Contributor

@filipemarins filipemarins Mar 7, 2022

Choose a reason for hiding this comment

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

Suggested change
const isEncryptedMessage = isE2EEMessage(message);
const encryptedMessageIsPending = isEncryptedMessage && message.e2e === 'pending';
const messageIsReady = !isEncryptedMessage || !encryptedMessageIsPending;
const isEncryptedMessage = isE2EEMessage(message);
const isEncryptedMessagePending = isEncryptedMessage && message.e2e === 'pending';
const isMessageReady = !isEncryptedMessage || !encryptedMessageIsPending;

I believe when the variable is boolean it is good to put the prefix is.


const mineUid = useUserId();

return (
Expand All @@ -103,7 +78,7 @@ const Message: FC<{ message: IMessage; sequential: boolean; subscription?: ISubs
data-username={user.username}
onClick={user.username !== undefined ? openUserCard(user.username) : undefined}
>
{(showRealName && user.name) || user.username}
{getUserDisplayName(user.name, user.username, showRealName)}
</MessageName>
{showUsername && (
<MessageUsername
Expand All @@ -113,16 +88,9 @@ const Message: FC<{ message: IMessage; sequential: boolean; subscription?: ISubs
@{user.username}
</MessageUsername>
)}
<MessageRoles>
{showRoles && Array.isArray(user.roles) && user.roles.length > 0 && (
<>
{user.roles.map((role, index) => (
<MessageRole key={index}>{role}</MessageRole>
))}
</>
)}
{message.bot && <MessageRole>{t('Bot')}</MessageRole>}
</MessageRoles>

<RolesList user={user} isBot={message.bot} />

<MessageTimestamp data-time={message.ts.toISOString()}>{formatters.messageHeader(message.ts)}</MessageTimestamp>
{message.private && (
// The MessageStatusPrivateIndicator component should not have name prop, it should be fixed on fuselage
Expand All @@ -131,14 +99,24 @@ const Message: FC<{ message: IMessage; sequential: boolean; subscription?: ISubs
<MessageIndicators message={message} />
</MessageHeader>
)}
<MessageBody>
{/* <MessageBody>
{message.e2e === 'pending'
? t('E2E_message_encrypted_placeholder')
: message.e2e !== 'done' &&
!message.blocks &&
message.md && <MessageBodyRender onMentionClick={openUserCard} mentions={message.mentions} tokens={message.md} />}
{!message.blocks && !message.md && message.msg}
{message.e2e === 'done' && message.msg}
</MessageBody> */}

<MessageBody>
{encryptedMessageIsPending && t('E2E_message_encrypted_placeholder')}

{messageIsReady && !message.blocks && message.md && (
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe some const to this case? like shoudShowMessageRender?

<MessageBodyRender onMentionClick={openUserCard} mentions={message.mentions} tokens={message.md} />
)}

{messageIsReady && !message.blocks && !message.md && message.msg}
</MessageBody>
{message.blocks && <MessageBlock mid={message._id} blocks={message.blocks} appId rid={message.rid} />}
{message.attachments && <Attachments attachments={message.attachments} file={message.file} />}
Expand All @@ -158,21 +136,8 @@ const Message: FC<{ message: IMessage; sequential: boolean; subscription?: ISubs
runAction={runActionLink(message)}
/>
)}
{message.reactions && Object.keys(message.reactions).length > 0 && (
<MessageReactions>
{Object.entries(message.reactions).map(([name, reactions]) => (
<MessageReaction
key={name}
counter={reactions.usernames.length}
hasReacted={hasReacted}
reactToMessage={reactToMessage}
name={name}
names={filterReactions(name)}
/>
))}
<MessageReactionAction onClick={openEmojiPicker} />
</MessageReactions>
)}

<ReactionsList message={message} />

{isThreadMainMessage(message) && (
<Thread
Expand Down Expand Up @@ -202,23 +167,9 @@ const Message: FC<{ message: IMessage; sequential: boolean; subscription?: ISubs
{message.location && <MessageLocation location={message.location} />}
{broadcast && user.username && <Broadcast replyBroadcast={replyBroadcast} mid={message._id} username={user.username} />}

{oembedIsEnabled && message.urls && (
<Box width={oembedWidth}>
<OEmbedList urls={message.urls} />
</Box>
)}
<PreviewList urls={message.urls} />

{isReadReceiptEnabled && (
<Box
position='absolute'
className={css`
top: 2px;
right: 0.5rem;
`}
>
<Icon name='check' size='x11' color='primary' />
</Box>
)}
<ReadReceipt />
</MessageContainer>
{!message.private && <Toolbox message={message} />}
</MessageTemplate>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React, { FC } from 'react';
import { IMessage, isEditedMessage } from '../../../../../definition/IMessage';
import { useTranslation } from '../../../../contexts/TranslationContext';
import { useUserId } from '../../../../contexts/UserContext';
import { isE2EEMessage } from '../../../../lib/isE2EEMessage';
import { useMessageDateFormatter, useShowStarred, useShowTranslated, useShowFollowing } from '../contexts/MessageListContext';

// edited() {
Expand Down Expand Up @@ -33,6 +34,7 @@ export const MessageIndicators: FC<{
const starred = useShowStarred({ message }); // TODO: useMessageStarred
const following = useShowFollowing({ message }); // TODO: useMessageFollowing

const isEncryptedMessage = isE2EEMessage(message);
const uid = useUserId();

const formatter = useMessageDateFormatter(); // TODO: useMessageDateFormatter
Expand Down Expand Up @@ -60,6 +62,8 @@ export const MessageIndicators: FC<{
)}
{message.pinned && <MessageStatusIndicatorItem name='pin' data-title={t('Message_has_been_pinned')} />}

{isEncryptedMessage && <MessageStatusIndicatorItem name='key' />}

{starred && <MessageStatusIndicatorItem name='star-filled' data-title={t('Message_has_been_starred')} />}
</MessageStatusIndicator>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const MessageReaction: FC<{
ref.current,
);
}}
onMouseLeave={() => {
onMouseLeave={(): void => {
closeTooltip();
}}
{...props}
Expand Down
35 changes: 35 additions & 0 deletions client/views/room/MessageList/components/MessageReactionsList.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { MessageReactions, MessageReactionAction } from '@rocket.chat/fuselage';
import React, { ReactElement } from 'react';

import { IMessage } from '../../../../../definition/IMessage';
import { useOpenEmojiPicker, useReactionsFilter, useReactToMessage, useUserHasReacted } from '../contexts/MessageListContext';
import { MessageReaction } from './MessageReaction';

const MessageReactionsList = ({ message }: { message: IMessage }): ReactElement | null => {
const hasReacted = useUserHasReacted(message);
const reactToMessage = useReactToMessage(message);
const filterReactions = useReactionsFilter(message);
const openEmojiPicker = useOpenEmojiPicker(message);

if (!message.reactions || !Object.keys(message.reactions).length) {
return null;
}
Copy link
Contributor

@filipemarins filipemarins Mar 7, 2022

Choose a reason for hiding this comment

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

This conditional should be on the parent component.

example:
Message.tsx

{Object.keys(message.reactions).length && <MessageReactionsList />}

This way the component will not be loaded.

https://dev.to/ucvdesh/stop-returning-null-components-312k


return (
<MessageReactions>
{Object.entries(message.reactions).map(([name, reactions]) => (
<MessageReaction
key={name}
counter={reactions.usernames.length}
hasReacted={hasReacted}
reactToMessage={reactToMessage}
name={name}
names={filterReactions(name)}
/>
))}
<MessageReactionAction onClick={openEmojiPicker} />
</MessageReactions>
);
};

export default MessageReactionsList;
27 changes: 27 additions & 0 deletions client/views/room/MessageList/components/MessageReadReceipt.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { css } from '@rocket.chat/css-in-js';
import { Box, Icon } from '@rocket.chat/fuselage';
import React, { ReactElement } from 'react';

import { useSetting } from '../../../../contexts/SettingsContext';

const MessageReadReceipt = (): ReactElement | null => {
const isReadReceiptEnabled = useSetting('Message_Read_Receipt_Enabled');
Copy link
Contributor

Choose a reason for hiding this comment

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


if (!isReadReceiptEnabled) {
return null;
}

return (
<Box
position='absolute'
className={css`
top: 2px;
right: 0.5rem;
`}
>
<Icon name='check' size='x11' color='primary' />
</Box>
);
};

export default MessageReadReceipt;
31 changes: 31 additions & 0 deletions client/views/room/MessageList/components/MessageRolesList.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { MessageRole, MessageRoles } from '@rocket.chat/fuselage';
import React, { ReactElement } from 'react';

import { useTranslation } from '../../../../contexts/TranslationContext';
import { UserPresence } from '../../../../lib/presence';
import { useMessageListShowRoles } from '../contexts/MessageListContext';

type MessageRolesListProps = {
user: UserPresence;
isBot?: boolean;
};

const MessageRolesList = ({ user, isBot }: MessageRolesListProps): ReactElement | null => {
const t = useTranslation();
const showRoles = useMessageListShowRoles();

if (!showRoles || !user.roles || !Array.isArray(user.roles) || user.roles.length < 1) {
return null;
}

return (
<MessageRoles>
{user.roles.map((role, index) => (
<MessageRole key={index}>{role}</MessageRole>
))}
{isBot && <MessageRole>{t('Bot')}</MessageRole>}
</MessageRoles>
);
};

export default MessageRolesList;
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { Box } from '@rocket.chat/fuselage';
import React, { ReactElement } from 'react';

import { useMessageOembedIsEnabled, useMessageOembedMaxWidth } from '../../../contexts/MessageContext';
import OEmbedResolver from './OEmbedResolver';
import UrlPreview from './UrlPreview';

Expand Down Expand Up @@ -38,7 +40,7 @@ export type UrlPreview = {
url: string;
};

type PreviewListProps = { urls: OembedUrlLegacy[] };
type PreviewListProps = { urls: OembedUrlLegacy[] | undefined };

type PreviewTypes = 'headers' | 'oembed';

Expand Down Expand Up @@ -105,18 +107,25 @@ const isPreviewData = (data: PreviewData | false): data is PreviewData => !!data

const isMetaPreview = (_data: PreviewData['data'], type: PreviewTypes): _data is PreviewMetadata => type === 'oembed';

const PreviewList = ({ urls }: PreviewListProps): ReactElement => {
const PreviewList = ({ urls }: PreviewListProps): ReactElement | null => {
const oembedIsEnabled = useMessageOembedIsEnabled();
const oembedWidth = useMessageOembedMaxWidth();

if (!oembedIsEnabled || !urls) {
return null;
}

const metaAndHeaders = urls.map(processMetaAndHeaders).filter(isPreviewData);

return (
<>
<Box width={oembedWidth}>
{metaAndHeaders.map(({ type, data }) => {
if (isMetaPreview(data, type)) {
return <OEmbedResolver meta={data} />;
}
return <UrlPreview {...data} />;
})}
</>
</Box>
);
};

Expand Down
3 changes: 2 additions & 1 deletion client/views/room/components/BlazeTemplate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ const BlazeTemplate: FC<
name: string;
} & Record<string, unknown>
> = ({ name, flexShrink, overflow, onClick, ...props }) => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const ref = useRef(null!);
useLayoutEffect(() => {
if (!ref.current || !Template[name]) {
return;
}

const view = Blaze.renderWithData(Template[name], props, ref.current);
return () => {
return (): void => {
view && Blaze.remove(view);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down