Page MenuHomePhabricator
Differential D6056

[web] intoduce MessageLikeTooltipButton component
ClosedPublic
Actions

Authored by ginsu on Dec 27 2022, 7:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 5:27 AM
Unknown Object (File)
Wed, Nov 13, 5:26 AM
Unknown Object (File)
Wed, Nov 13, 5:26 AM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
atul
tomek
Commits
rCOMMac5fb78605ad: [web] intoduce MessageLikeTooltipButton component
Summary

introduce MessageLikeTooltipButton component. This component returns 👍 wrapped in a div, and the class of the div changes depending on the viewerReacted prop that gets passed down. I imagine that this will be a temporary component that will eventually be deleted once we extend from message liking to message reactions

Depends on D6055

Linear Task: ENG-2474

Test Plan

Please look at the screenshots to see the MessageLikeTooltipButton component:

When viewerReacted is true:

Screenshot 2022-12-27 at 1.24.52 PM.png (680×1 px, 91 KB)

When viewerReacted is false:

Screenshot 2022-12-27 at 1.25.03 PM.png (802×1 px, 94 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu created this revision.Dec 27 2022, 7:28 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 27 2022, 7:28 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 27 2022, 7:31 AM
Harbormaster failed remote builds in B14803: Diff 20189!
ginsu edited the summary of this revision. (Show Details)Dec 27 2022, 10:29 AM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, tomek.
ginsu added a parent revision: D6055: [web] introduce useOnClickReact hook in reaction message utils.
ginsu attached a referenced file: F306500: Screenshot 2022-12-27 at 1.25.03 PM.png. (Show Details)
ginsu attached a referenced file: F306501: Screenshot 2022-12-27 at 1.24.52 PM.png. (Show Details)
ginsu updated this revision to Diff 20195.Dec 27 2022, 10:36 AM

rebase

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 27 2022, 10:39 AM
Harbormaster failed remote builds in B14806: Diff 20195!
ginsu updated this revision to Diff 20196.Dec 27 2022, 10:43 AM

fix imports

ginsu added a child revision: D6057: [web] introduce message like button to message tooltip.Dec 27 2022, 10:49 AM
Harbormaster completed remote builds in B14807: Diff 20196.Dec 27 2022, 10:57 AM
ginsu requested review of this revision.Dec 27 2022, 10:57 AM
tomek requested changes to this revision.Dec 29 2022, 5:13 AM
tomek added inline comments.
web/chat/message-like-tooltip-button.css
7–9 ↗(On Diff #20196)

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?

This revision now requires changes to proceed.Dec 29 2022, 5:13 AM
ginsu requested review of this revision.Dec 29 2022, 7:55 AM
ginsu added inline comments.
web/chat/message-like-tooltip-button.css
7–9 ↗(On Diff #20196)

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

tomek requested changes to this revision.Dec 29 2022, 8:03 AM
tomek added inline comments.
web/chat/message-like-tooltip-button.css
7–9 ↗(On Diff #20196)

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.

This revision now requires changes to proceed.Dec 29 2022, 8:03 AM
ginsu added inline comments.Dec 29 2022, 8:05 AM
web/chat/message-like-tooltip-button.css
7–9 ↗(On Diff #20196)

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

ginsu updated this revision to Diff 20350.Dec 29 2022, 8:46 AM

addressed tomeks comments

Harbormaster completed remote builds in B14932: Diff 20350.Dec 29 2022, 9:00 AM
atul accepted this revision.Dec 29 2022, 12:50 PM

Looks good, one question inline

web/chat/message-tooltip.css
28–29 ↗(On Diff #20350)

Should this be

ginsu updated this revision to Diff 20392.Dec 29 2022, 1:27 PM

correct css

Harbormaster completed remote builds in B14948: Diff 20392.Dec 29 2022, 1:41 PM
tomek accepted this revision.Dec 30 2022, 5:55 AM
tomek added inline comments.
web/chat/message-like-tooltip-button.css
4 ↗(On Diff #20392)

We shouldn't use colors directly. In theme.css there are two sections: one with color definitions and one where we match colors with component names. We should introduce a new variable to the second sections and use here.

This revision is now accepted and ready to land.Dec 30 2022, 5:55 AM
ginsu updated this revision to Diff 20449.Dec 30 2022, 11:34 AM

address tomek's feedback

Harbormaster completed remote builds in B14982: Diff 20449.Dec 30 2022, 11:49 AM
Closed by commit rCOMMac5fb78605ad: [web] intoduce MessageLikeTooltipButton component (authored by ginsu). · Explain WhyDec 30 2022, 12:03 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMMac5fb78605ad: [web] intoduce MessageLikeTooltipButton component.

Revision Contents
Changeset List

Diff 20451

View Options

web/chat/message-like-tooltip-button.css

Loading...
View Options

web/chat/message-like-tooltip-button.react.js

Loading...
View Options

web/chat/message-tooltip.css

Loading...
View Options

web/theme.css

Loading...