Page MenuHomePhabricator
Differential D4705

[web] Finish styling `MessageTimestampTooltip`
ClosedPublic
Actions

Authored by • abosh on Aug 1 2022, 1:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 7:23 PM
Unknown Object (File)
Wed, Apr 2, 4:21 AM
Unknown Object (File)
Sun, Mar 30, 11:08 AM
Unknown Object (File)
Sat, Mar 29, 10:19 AM
Unknown Object (File)
Thu, Mar 20, 9:42 PM
Unknown Object (File)
Sat, Mar 15, 7:07 PM
Unknown Object (File)
Mar 5 2025, 12:24 PM
Unknown Object (File)
Mar 5 2025, 10:55 AM
View All Files
Subscribers
• adrian
ashoat

Details

Reviewers
atul
• jacek
tomek
Commits
rCOMM33347778d029: [web] Finish styling `MessageTimestampTooltip`
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

• abosh created this revision.Aug 1 2022, 1:48 PM
Herald added subscribers: • adrian, ashoat. · View Herald TranscriptAug 1 2022, 1:48 PM
• abosh edited the test plan for this revision. (Show Details)Aug 1 2022, 1:51 PM
Harbormaster completed remote builds in B11036: Diff 15178.Aug 1 2022, 1:58 PM
• abosh requested review of this revision.Aug 1 2022, 1:58 PM
• abosh updated this revision to Diff 15182.EditedAug 1 2022, 7:03 PM

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.

Harbormaster completed remote builds in B11040: Diff 15182.Aug 1 2022, 7:15 PM
• jacek accepted this revision.Aug 2 2022, 1:06 AM
• 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 marked an inline comment as done.Aug 2 2022, 8:44 AM
• abosh added inline comments.
web/chat/tooltip.react.js
71 ↗(On Diff #15182)

Great catch!

• abosh updated this revision to Diff 15215.Aug 2 2022, 8:45 AM
• 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)Aug 2 2022, 8:50 AM
• abosh edited the test plan for this revision. (Show Details)
• abosh added 2 blocking reviewer(s): atul, tomek.Aug 2 2022, 8:55 AM

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
Harbormaster completed remote builds in B11053: Diff 15215.Aug 2 2022, 8:55 AM
atul accepted this revision.Aug 4 2022, 10:22 AM
• abosh removed 1 blocking reviewer(s): tomek.Aug 4 2022, 10:29 AM
This revision is now accepted and ready to land.Aug 4 2022, 10:29 AM
Closed by commit rCOMM33347778d029: [web] Finish styling `MessageTimestampTooltip` (authored by • abosh). · Explain WhyAug 4 2022, 10:45 AM
This revision was automatically updated to reflect the committed changes.
• abosh added a commit: rCOMM33347778d029: [web] Finish styling `MessageTimestampTooltip`.

Revision Contents
Changeset List

PathSize
web/
chat/
12 lines
7 lines

Diff 15324

View Options

web/chat/tooltip.css

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

web/chat/tooltip.react.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes