[web] Introduce and style `messageActionContainer` for `ComposedMessage`
ClosedPublic
Actions

Authored by atul on Feb 23 2022, 8:31 PM.

Details

Reviewers

tomek
• jacek
• benschac
varun
ashoat

Commits

rCOMM8ac1900c521b: [web] Introduce and style `messageActionContainer` for `ComposedMessage`

Summary

Before: https://blob.sh/atul/36ae.png
After: https://blob.sh/atul/5b10.png

Illustration of messageActionActiveArea: https://blob.sh/atul/aa2b.png

Test Plan

Diff Detail

Repository

rCOMM Comm

Branch

landfeb25 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Feb 23 2022, 8:31 PM

Herald added subscribers: • karol, • adrian, ashoat. · View Herald TranscriptFeb 23 2022, 8:31 PM

atul published this revision for review.Feb 23 2022, 8:33 PM

Harbormaster completed remote builds in B7004: Diff 9875.Feb 23 2022, 8:36 PM

small nits, other wise this looks good.

web/chat/chat-message-list.css
109 ↗	(On Diff #9875)	nit: `padding: 0 6px`
111 ↗	(On Diff #9875)	Can we use a theme variable here rather than hard coding a color? If you look at `theme.css` you'll see how we've done it so far.

This revision is now accepted and ready to land.Feb 24 2022, 3:48 AM

Herald added a reviewer: ashoat. · View Herald TranscriptFeb 24 2022, 3:48 AM

This revision now requires review to proceed.Feb 24 2022, 3:48 AM

tomek requested changes to this revision.Feb 24 2022, 8:22 AM

tomek added inline comments.

web/chat/chat-message-list.css
109 ↗	(On Diff #9875)	Not sure how closely do we follow figma, but we have 8px padding there it might be the case that other padding and margins make this look the same as there - please check this

This revision now requires changes to proceed.Feb 24 2022, 8:22 AM

ashoat removed a reviewer: ashoat.Feb 24 2022, 10:44 AM

atul requested review of this revision.Feb 24 2022, 11:13 AM

atul added inline comments.

web/chat/chat-message-list.css
109 ↗	(On Diff #9875)	The padding numbers are going to be off for all of these diffs while the icons lack their own padding. Between the padding/margin in `div.messageActionContainer` and the padding/margin in `div.messageActionContainer svg`, it looks close to what is in Figma.

• benschac requested changes to this revision.Feb 24 2022, 11:27 AM

• benschac added inline comments.

web/chat/chat-message-list.css
111 ↗	(On Diff #9875)	Could you add something like `---message-container-bg: var(--shades-black-90)` to the `theme.css` file?

This revision now requires changes to proceed.Feb 24 2022, 11:27 AM

atul requested review of this revision.Feb 25 2022, 8:37 AM

address feedback

Harbormaster completed remote builds in B7044: Diff 9928.Feb 25 2022, 8:55 AM

Looks good to me. Next time could you also link the figma screen? Assuming this is correct: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A77706

web/chat/chat-message-list.css
109–110 ↗	(On Diff #9928)

• benschac added inline comments.Feb 25 2022, 10:51 AM

web/chat/chat-message-list.css
109–110 ↗	(On Diff #9928)	ment to say nit: use short hand for something like this. First time using the suggest feature. Sorry about that.

ashoat accepted this revision.Feb 25 2022, 12:59 PM

address feedback about padding shorthand

Harbormaster completed remote builds in B7052: Diff 9938.Feb 25 2022, 1:30 PM

(Note: Landing this diff even though @palys-swm requested changes on an earlier revision... I think I addressed his concerns and it's the weekend in Poland. Happy to address any further concerns in a separate diff.)

This revision was not accepted when it landed; it landed in state Needs Review.Feb 25 2022, 2:52 PM

Closed by commit rCOMM8ac1900c521b: [web] Introduce and style `messageActionContainer` for `ComposedMessage` (authored by atul). · Explain Why

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMM8ac1900c521b: [web] Introduce and style `messageActionContainer` for `ComposedMessage`.