Page MenuHomePhabricator

[web] Finish styling `MessageTimestampTooltip`
ClosedPublic

Authored by abosh on Aug 1 2022, 1:48 PM.
Tags
None
Referenced Files
F2156977: D4705.id15215.diff
Mon, Jul 1, 12:55 AM
F2155734: D4705.id15182.diff
Sun, Jun 30, 10:19 PM
F2149366: D4705.id.diff
Sun, Jun 30, 7:51 AM
Unknown Object (File)
Thu, Jun 27, 3:44 AM
Unknown Object (File)
Wed, Jun 26, 5:26 AM
Unknown Object (File)
Mon, Jun 24, 5:04 PM
Unknown Object (File)
Thu, Jun 13, 3:59 PM
Unknown Object (File)
Wed, Jun 12, 8:25 AM
Subscribers

Details

Summary

Relevant Linear issue here.

This diff adds styling changes to MessageTimestampTooltip, specifically changing the color of the tooltip, increasing the width of the tooltip (so it is at minimum the width of the MessageTooltip with the reply/create thread buttons), some padding, and makes the border radius match the MessageTooltip.

Depends on D4704.

Test Plan

Tested on Chrome/Safari and looks as expected:
Chrome:

Safari:

Additional stills:

image.png (312×702 px, 24 KB)

image.png (180×380 px, 16 KB)

image.png (250×1 px, 45 KB)

image.png (508×1 px, 128 KB)

Finally, the timestamp does not appear behind photos, as @ashoat noted on Linear:

image.png (1×2 px, 4 MB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Un-modify --tool-tip-bg in theme.css. Since we want the MessageTimestampTooltip color to match MessageTooltip, we can just set background-color to --message-action-tooltip-bg, as done in line 44 of tooltip.css of this diff, since --message-action-tooltip-bg sets the color of MessageTooltip.

jacek added inline comments.
web/chat/tooltip.react.js
71 ↗(On Diff #15182)

I think it would be better to put the new class into existing classNames in useMemo in line 63. Now, className here is calculated every render.

This revision is now accepted and ready to land.Aug 2 2022, 1:06 AM
abosh added inline comments.
web/chat/tooltip.react.js
71 ↗(On Diff #15182)

Great catch!

abosh marked an inline comment as done.

Put .messageTimestampTooltip CSS class in the useMemo on line 62 so it is memoized, per @jacek's feedback.

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

Adding @tomek and @atul as blocking to get at least one more set of eyes on this, although the rest of the stack prior to this diff still needs to be reviewed.

This revision now requires review to proceed.Aug 2 2022, 8:55 AM
This revision is now accepted and ready to land.Aug 4 2022, 10:29 AM