feat: Improved delivered status indicator WPB-9170 #17553

svitovyda · 2024-06-10T19:32:04Z

WPB-9170 Improve hover states and paddings for delivered status

Description

Now the conversation should have additional empty space at the right, and the last delivered message should contain a translated "Delivered" indicator, visible always (instead of on mouse over).
This feature design was made considering that most of the webapp/desktop users use it on res 800px+, design improvements for smaller screens will be defined later.
After this version is deployed, we might implement another design with checkboxes (under a separate ticket) that can be toggled using debug

Note 1: for the first implementation, where each message had an additional div to the right: I assumed that any message header would be short enough to not wrap it into the container that takes 100% - ${deliveredIndicatorWidth}px. It might be the improvement for the next design iteration to address the small screen look

Note 2: for the second implementation, where the entire MessageList has a margin: Now the MessageList has to add and immediately remove the "Delivered" indicator to identify its width. Another option could be to add a static margin (of ~100px) and listen to the message components when one of them will have to add this indicator, then adjust the margin width. Another option could be that each message component could have a margin, but I'm not sure what we will win here

Screenshots/Screencast (for UI changes)

Checklist

PR has been self reviewed by the author;
Hard-to-understand areas of the code have been commented;
If it is a core feature, unit tests have been added;

svitovyda · 2024-06-10T19:39:36Z

src/script/components/MessagesList/Message/DeliveredIndicator/DeliveredIndicator.styles.ts

+  fontSize: 'var(--font-size-small)',
+  fontWeight: 'var(--font-weight-regular)',
+  wordWrap: 'normal',
+  width: 'min-content',


One translation of "Delivered" is two words, here I make sure they will be wrapped

svitovyda · 2024-06-10T19:40:08Z

src/script/components/MessagesList/Message/DeliveredIndicator/DeliveredIndicator.styles.ts

+  boxSizing: 'border-box',
+  justifyContent: 'center',
+  alignItems: 'center',
+  position: 'absolute',


It's another self-positioned component

svitovyda · 2024-06-10T19:42:32Z

src/script/components/MessagesList/Message/ReadReceiptStatus.tsx

-  isLastDeliveredMessage,
-  onClickDetails,
-}: ReadReceiptStatusProps) => {
+export const ReadReceiptStatus = ({message, is1to1Conversation, onClickDetails}: ReadReceiptStatusProps) => {


This one can be moved to the ReadIndicator folder, which will make it easier to not forget about one of them when the other should be changed

svitovyda · 2024-06-10T19:44:11Z

src/style/components/asset/file-asset.less

@@ -22,7 +22,7 @@
  width: 100%;
  flex-wrap: wrap;
  align-self: center;
-  padding: 12px;
+  padding: 12px 0 12px 12px;


Since the general designer's idea was to introduce additional whitespace at the right of the conversation, I guess we don't need the right padding of the message bodies

svitovyda · 2024-06-10T19:50:46Z

src/script/components/MessagesList/Message/ContentMessage/ContentMessage.tsx

-        })}
-        {...(ephemeralCaption && {title: ephemeralCaption})}
+        aria-label={messageAriaLabel}
+        className="content-message-wrapper"


adding here className={cx('content-message-wrapper', { 'width': `calc(100% - ${deliveryIndicatorRef?.offsetWidth || 0}px)` })} doesn't work

svitovyda · 2024-06-10T19:56:30Z

src/script/components/MessagesList/Message/ContentMessage/ContentMessage.tsx

+        aria-label={messageAriaLabel}
+        className="content-message-wrapper"
+        css={{
+          width: `calc(100% - ${deliveryIndicatorRef?.offsetWidth || 0}px)`,


I tried playing with display: flex and overflow etc. and with some of the message content (images, pre) the message was stretching outside of the screen

svitovyda · 2024-06-10T19:59:45Z

src/script/components/MessagesList/Message/ContentMessage/ContentMessage.tsx

-        />
-      )}
-    </div>
+      <DeliveredIndicator


Another option was to add it on the level of the MessageWrapper, but I'm not sure it won't break again the stretching of the message for content like pre, and maybe we will need completely different behaviour for different kinds of messages. Also, it avoids additional div containers

svitovyda · 2024-06-10T20:05:17Z

src/script/components/MessagesList/Message/DeliveredIndicator/DeliveredIndicator.styles.ts

+  fontWeight: 'var(--font-weight-regular)',
+  wordWrap: 'normal',
+  width: 'min-content',
+  visibility: isLastDeliveredMessage ? 'unset' : 'hidden',


I don't know how else to add the empty space of the dynamic width (the width of the translated longest word of "Delivered" plus padding). I guess it is possible to set the width once for all somewhere globally, even set the global right margin for the messages container, and only add one span for the last delivered message.

svitovyda · 2024-06-10T20:06:58Z

src/style/content/conversation/message-list.less

@@ -274,7 +274,6 @@
 // MESSAGE - BODY
 .message-body {
  position: relative;
-  padding-right: 40px;


Same here - we already have one big whitespace at the right, we don't need this one anymore

tlebon · 2024-06-11T11:37:58Z

regarding this image you shared -> i think the delivered status should be aligned with the time stamp at the top of longer mesages. (also timestamp should not wrap)

codecov-commenter · 2024-06-11T14:47:45Z

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 46.13%. Comparing base (c0c9c7f) to head (450d23b).
Report is 36 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev   #17553      +/-   ##
==========================================
+ Coverage   46.11%   46.13%   +0.01%     
==========================================
  Files         760      762       +2     
  Lines       25016    25025       +9     
  Branches     5721     5722       +1     
==========================================
+ Hits        11537    11546       +9     
+ Misses      12036    12035       -1     
- Partials     1443     1444       +1

svitovyda · 2024-06-11T18:43:05Z

regarding this image you shared -> i think the delivered status should be aligned with the time stamp at the top of longer mesages. (also timestamp should not wrap)

I now aligned "Delivered" text to the top of the message, and also made the timestamp text nowrap (it was broken before my changes). And aligned the MessageActions to the most right - 16 px.

svitovyda · 2024-06-12T09:18:45Z

...cript/components/MessagesList/Message/ContentMessage/MessageActions/MessageActions.styles.ts

  display: 'inline-flex',
  alignItems: 'center',
  minHeight: '32px',
  minWidth: '40px',
  position: 'absolute',
-  right: '16px',
+  right: `${16 - rightMarginWidth}px`,


I need to position reactions menu to look in the old way - 16px from the right of the screen, not of the message container. I can't use position: fixed as I need it only horizontally, and vertically it should still be relative to the parent, to be scrolled. I also tried to play with left: (100vw - 100% - 16px), but 100% is a message container, already resized to the right margin of the MessageList container with margin, so it gives the same result as right: 16px.

svitovyda · 2024-06-12T09:26:04Z

src/script/components/MessagesList/Message/ContentMessage/ContentMessage.tsx

  message: ContentMessage;
  onClickButton: (message: CompositeMessage, buttonId: string) => void;
  onRetry: (message: ContentMessage) => void;
  quotedMessage?: ContentMessage;
  selfId: QualifiedId;
  isMsgElementsFocusable: boolean;
  onClickReaction: (emoji: string) => void;
+  rightMarginWidth: number;


In this implementation, rightMarginWidth gets calculated in MessageList and must be used to correctly position MessageActionsMenu (in old way, aligned to the screen, ignoring the new margin). It can be made as a context to not do this long chain-propsing

svitovyda · 2024-06-12T09:55:20Z

src/script/components/MessagesList/MessageList.tsx

  return (
    <>
+      {rightMarginWidth === 0 && <DeliveredIndicator ref={setDeliveredIndicatorWidth} isLastDeliveredMessage={false} />}


If we agree on this approach, it can be modified to

if(rightMarginWidth === 0) return <DeliveredIndicator ref={setDeliveredIndicatorWidth} isLastDeliveredMessage={false} /> return ( ...

to avoid one extra render. Then the MessageList.test must be adjusted to ignore the first render

svitovyda · 2024-06-12T09:57:22Z

src/script/components/MessagesList/MessageList.tsx

@@ -121,6 +122,8 @@ export const MessagesList: FC<MessagesListParams> = ({
  const [highlightedMessage, setHighlightedMessage] = useState<string | undefined>(conversation.initialMessage()?.id);
  const conversationLastReadTimestamp = useRef(conversation.last_read_timestamp());

+  const [rightMarginWidth, setRightMarginWidth] = useState<number>(0);


This also can be ref, to avoid each time getting recalculated and re-rendered all the time, but should be recalculated on the language switch.

sonarcloud · 2024-06-12T10:01:01Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

otto-the-bot added type: feature / request ✨ 👕 size: L labels Jun 10, 2024

svitovyda commented Jun 10, 2024

View reviewed changes

svitovyda force-pushed the feat/delivered-status-WPB-9170 branch from 0e07132 to 37704a7 Compare June 11, 2024 13:06

svitovyda added 3 commits June 11, 2024 20:38

feat: Improved delivered status indicator WPB-9170

6501a37

Fix tests and add unit test for DeliveredIndicator

87105b3

Make sure read indicator doesn't get wrapped

cfac804

svitovyda force-pushed the feat/delivered-status-WPB-9170 branch from 09c9a8e to b29dcc4 Compare June 11, 2024 18:38

svitovyda force-pushed the feat/delivered-status-WPB-9170 branch from b29dcc4 to cd20f5a Compare June 11, 2024 18:54

svitovyda commented Jun 12, 2024

View reviewed changes

svitovyda force-pushed the feat/delivered-status-WPB-9170 branch from cd20f5a to 5127b89 Compare June 12, 2024 09:19

svitovyda commented Jun 12, 2024

View reviewed changes

Move "right space" width logic to the MessageList

450d23b

svitovyda force-pushed the feat/delivered-status-WPB-9170 branch from 5127b89 to 450d23b Compare June 12, 2024 09:58

svitovyda marked this pull request as ready for review June 12, 2024 09:58

svitovyda requested review from otto-the-bot and a team as code owners June 12, 2024 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improved delivered status indicator WPB-9170 #17553

feat: Improved delivered status indicator WPB-9170 #17553

svitovyda commented Jun 10, 2024 •

edited by github-actions bot

Loading

svitovyda Jun 10, 2024

svitovyda Jun 10, 2024

svitovyda Jun 10, 2024 •

edited

Loading

svitovyda Jun 10, 2024

svitovyda Jun 10, 2024 •

edited

Loading

svitovyda Jun 10, 2024

svitovyda Jun 10, 2024

svitovyda Jun 10, 2024

svitovyda Jun 10, 2024

tlebon commented Jun 11, 2024

codecov-commenter commented Jun 11, 2024 •

edited

Loading

svitovyda commented Jun 11, 2024

svitovyda Jun 12, 2024 •

edited

Loading

svitovyda Jun 12, 2024

svitovyda Jun 12, 2024

svitovyda Jun 12, 2024

sonarcloud bot commented Jun 12, 2024

feat: Improved delivered status indicator WPB-9170 #17553

Are you sure you want to change the base?

feat: Improved delivered status indicator WPB-9170 #17553

Conversation

svitovyda commented Jun 10, 2024 • edited by github-actions bot Loading

Description

Screenshots/Screencast (for UI changes)

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svitovyda Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svitovyda Jun 10, 2024 • edited Loading

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

tlebon commented Jun 11, 2024

codecov-commenter commented Jun 11, 2024 • edited Loading

Codecov Report

svitovyda commented Jun 11, 2024

svitovyda Jun 12, 2024 • edited Loading

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

sonarcloud bot commented Jun 12, 2024

Quality Gate passed

svitovyda commented Jun 10, 2024 •

edited by github-actions bot

Loading

svitovyda Jun 10, 2024 •

edited

Loading

svitovyda Jun 10, 2024 •

edited

Loading

codecov-commenter commented Jun 11, 2024 •

edited

Loading

svitovyda Jun 12, 2024 •

edited

Loading