[web] Reintroduce `MessageTimestampTooltip` in `ChatMessageList`
ClosedPublic
Actions

Authored by • abosh on Jul 7 2022, 10:41 AM.

Details

Reviewers

tomek
• jacek
atul

Commits

rCOMM2c7dbf3314ec: [web] Reintroduce `MessageTimestampTooltip` in `ChatMessageList`

Summary

This diff is the first part in a stack of commits that will work to fix the timestamp bug outlined in this Linear issue. Also see @ashoat's comment on D3567 for more context.

This diff removes the timestamp from MessageTooltip and instead reintroduces MessageTimestampTooltip in ChatMessageList, thus reverting the visual aspect of this change to what the timestamp tooltip used to look like before MessageActionButtons was introduced.

The reason this was done is:

Start the new stack of diffs to fix the timestamp bug
By moving the timestamp out of MessageTooltip, the component is no longer inside the messageContainer in ChatMessageList which has a overflow-y: auto property. This overflow-y property was causing the timestamp to be cut off within the ChatMessageList container since it did not allow any elements to extend beyond its perimeter.
This reverts the code to a familiar place that we have seen before. This diff should be fairly easy to review -- although there are added files, these are mostly just copied over from the old version of the tooltip/timestamp and the front-end changes are identical.

Test Plan

Tested on Chrome/Safari and looks as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• abosh created this revision.Jul 7 2022, 10:41 AM

Herald added a subscriber: • adrian. · View Herald TranscriptJul 7 2022, 10:41 AM

• abosh edited the summary of this revision. (Show Details)Jul 7 2022, 10:44 AM

• abosh edited the test plan for this revision. (Show Details)

• abosh added inline comments.Jul 7 2022, 10:48 AM

web/chat/message-timestamp-tooltip.react.js
33 ↗	(On Diff #14290)	This file was mostly copied over from the version of the codebase prior to D3567, with a few changes (like making a new CSS file for this component specifically, see line 9).
web/chat/message-tooltip.css
12–29 ↗	(On Diff #14290)	These were for the old `timestamp` in `MessageTooltip` which was deleted in this diff. However, in the future the timestamp and action buttons could be put back into the same component (that is outside the `messageContainer` in `ChatMessageList` to escape the `overflow-y: auto` property), but that will be handled in later diffs.

Harbormaster completed remote builds in B10380: Diff 14290.Jul 7 2022, 10:54 AM

• abosh requested review of this revision.Jul 7 2022, 10:54 AM

Resigning and adding @ashoat as blocking review since he has far more context on tooltip positioning and the layout nuances involved.

If resigning and adding @ashoat isn't the right thing to do here, feel free to add me back as blocking reviewer and I'll take a closer look.

This diff is the first part in a stack of commits that will work to fix the timestamp bug

Makes sense, but we should avoid landing this one until the whole stack is up / accepted, since it will otherwise result in a regression (if I'm not mistaken).

This revision is now accepted and ready to land.Jul 17 2022, 1:12 PM

• abosh mentioned this in D4700: [web] Remove `connectDropTarget` from `ChatMessageList` with new `tooltip` changes.Aug 1 2022, 9:29 AM

• abosh added a child revision: D4700: [web] Remove `connectDropTarget` from `ChatMessageList` with new `tooltip` changes.Aug 1 2022, 12:08 PM

In D4434, there were certain changes made that made D4481 broken since ChatMessageList was refactored. This update brings ChatMessageList up to date and out of a broken state. Since this is the first in a stack of diffs that will resolve the timestamp bug (Linear issue here), it doesn't need to be landed yet since I plan on getting the rest of the diffs up. I have abandoned D4700 since those changes have been made as an update in here.

• abosh added a reviewer: atul.Aug 1 2022, 12:24 PM

Harbormaster completed remote builds in B11021: Diff 15162.Aug 1 2022, 12:37 PM

• abosh added a child revision: D4704: [web] Align `MessageTimestampTooltip` under `MessageTooltip`.Aug 1 2022, 1:02 PM

Re-requesting review since the entire stack for the new MessageTimestampTooltip has been pushed, and this diff was only amended to work with the changes made in D4434. Removing @ashoat since he already accepted the old code/has context on this issue and probably doesn't need to be a first-pass reviewer on this stack.

tomek accepted this revision.Aug 4 2022, 6:59 AM

This revision is now accepted and ready to land.Aug 4 2022, 6:59 AM

Adding @atul and @jacek as blocking to get at least one more reviewer on this.

This revision now requires review to proceed.Aug 4 2022, 7:42 AM

atul accepted this revision.Aug 4 2022, 8:59 AM

atul added inline comments.

web/chat/message-timestamp-tooltip.react.js
36 ↗	(On Diff #15162)	We typically don't want to leave `console.log(...)` statements in the codebase. Was this intentional, or was it just included for whatever debugging/troubleshooting purposes as you were working?