Page MenuHomePhabricator

[web] Remove `MessageTimestampTooltip` from `ChatMessageList`
ClosedPublic

Authored by atul on Mar 30 2022, 10:48 AM.
Tags
None
Referenced Files
F2163682: D3567.diff
Mon, Jul 1, 8:46 PM
F2163256: D3567.diff
Mon, Jul 1, 7:41 PM
Unknown Object (File)
Sun, Jun 23, 1:11 PM
Unknown Object (File)
Sun, Jun 23, 1:11 PM
Unknown Object (File)
Sun, Jun 23, 1:11 PM
Unknown Object (File)
Sun, Jun 23, 1:02 PM
Unknown Object (File)
Tue, Jun 18, 8:07 AM
Unknown Object (File)
Tue, Jun 18, 8:07 AM

Details

Summary

Remove the one usage of MessageTimestampTooltip in web.

Next diffs:

  1. Remove MessageTimestampTooltip component and associated styles.
  2. Display timestamp as part of MessageActionButtons component.
Test Plan
  1. Made sure there were no other usages of the MessageTimestampTooltip component.
  2. Made sure that the timestamp tooltip was no longer displayed when hovering over a message.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/chat/chat-message-list.react.js
266 ↗(On Diff #10828)

this.state.mouseOverMessagePosition is passed as a prop to the Message component, so there's nothing else to remove in this file related to mouseOverPosition

270 ↗(On Diff #10828)

this.props.timeZone is also passed as a prop to the Message component from ChatMessageList so there's nothing else to remove in this file related to it

atul published this revision for review.Mar 30 2022, 10:51 AM
This revision is now accepted and ready to land.Mar 30 2022, 11:04 PM
This revision was landed with ongoing or failed builds.Mar 31 2022, 9:30 AM
This revision was automatically updated to reflect the committed changes.

This diff was a mistake and ultimately what led to ENG-1266. In fact, there is no way to get an element that appears inside an scroll area (eg. overflow: auto) to overflow the bounds of that scroll area without using position: fixed. In other words, we can't simultaneously (a) position the tooltip relative to a message, while (b) making it possible for that tooltip to appear outside the bounds of the scroll area that contains that message.

The reason we initially implemented the timestamp tooltip this way was in fact exactly because of this reason. After spending some time with @yayabosh investigating this today, the memory of initially working through this with @KatPo rushed back to me. Unfortunately I did not recall that initial work when reviewing this diff.

@yayabosh is working on reintroducing this code – we will once again need to store the coordinates of where the tooltip appears in some React state, and make sure the tooltip is rendered outside the bounds of the scroll area.

When we do reintroduce the code, we should make sure to document (in a code comment) *why* we're doing things this way, so that somebody doesn't come back and naively revert it again.