Paths

Table of Contentst

[web] [fix] style mark as unread
ClosedPublic
Actions

Authored by • benschac on Mar 14 2022, 9:40 AM.

Details

Reviewers

atul
tomek
• jacek
ashoat

Commits

rCOMM216d3994bfb1: [web] [fix] style mark as unread

Summary

update mark as read. https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1085%3A85147

We decided to switch the background color for hover state mark as unread button.

Image 2022-03-22 at 9.35.04 AM.jpg (910×893 px, 218 KB)

to option one.
the selected chat thread item (mochi) is the selected state which is not changing from the design.
the reason we needed to update the background color for the hover state mark as unread button is because the background color of hover chat thread item and background color of mark as unread button are the same color in the figma file.

Test Plan

check figma. We still have similar issues around the hover state leaving too soon on mouse leave which need to be fixed in another diff. This is just updating the styling to match our spec.

Diff Detail

Repository

rCOMM Comm

Branch

mark-as-unread

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• benschac created this revision.Mar 14 2022, 9:40 AM

Herald added subscribers: • karol, • adrian, tomek, ashoat. · View Herald TranscriptMar 14 2022, 9:40 AM

• benschac added a parent revision: D3421: [web] [fix] hover state background color on thread list.Mar 14 2022, 9:42 AM

• benschac edited the summary of this revision. (Show Details)

• benschac added reviewers: tomek, • jacek.Mar 14 2022, 9:44 AM

Harbormaster completed remote builds in B7340: Diff 10341.Mar 14 2022, 9:48 AM

• benschac requested review of this revision.Mar 14 2022, 9:48 AM

Accepting.. but one question inline about an extraneous onHover style.

web/chat/chat-thread-list-item-menu.css
61–63	Why do we need this if we aren't changing the color on hover? It looks like it's `--chat-thread-list-menu-bg` for both the hover and non hover state?

This revision is now accepted and ready to land.Mar 14 2022, 5:32 PM

• benschac added inline comments.Mar 15 2022, 6:49 AM

web/chat/chat-thread-list-item-menu.css
61–63	We don't need it, removing. It was laying around before and I just updated it because I saw it. You're right.

remove css

Harbormaster completed remote builds in B7372: Diff 10379.Mar 15 2022, 6:54 AM

@ashoat -- updated colors of the mark as unread button based off of D3421 changes to hover and selected state. We currently don't have design for mark as unread (because the previous diff flips the hover and selection colors from what's in figma).

In the screenshot below there are two options that looked reasonable. IMO I think option 1 looks better.

Curious what your thoughts are? I can update this diff with the updated mark as unread background color after we've decided on a background color.

before:

after:

@ashoat - no code changes. Just putting this in your queue to get input on the updated background color options.

• benschac mentioned this in D3421: [web] [fix] hover state background color on thread list.Mar 22 2022, 7:06 AM

I agree, option 1 looks best!

(Just to be clear, this diff isn't in my queue since I'm not on the review)

• benschac added a reviewer: ashoat.Mar 22 2022, 11:51 AM

• benschac edited the summary of this revision. (Show Details)Mar 22 2022, 11:54 AM

• benschac edited the summary of this revision. (Show Details)

• benschac edited the summary of this revision. (Show Details)Mar 22 2022, 11:58 AM

• benschac edited the summary of this revision. (Show Details)

Not super clear why I'm being added to the review... I thought you just wanted my opinion on the different designs, which I gave above.

In the future, can you please make your intentions clear for short-circuiting the standard review process? There are plenty of good reasons to do this, but I can't tell what the reason is here.

web/chat/chat-thread-list-item-menu.css
44 ↗	(On Diff #10379)	There's an extraneous space here

This revision is now accepted and ready to land.Mar 22 2022, 9:54 PM

In D3422#94966, @ashoat wrote:

Not super clear why I'm being added to the review... I thought you just wanted my opinion on the different designs, which I gave above.

In the future, can you please make your intentions clear for short-circuiting the standard review process? There are plenty of good reasons to do this, but I can't tell what the reason is here.

I wanted to make sure it was in your queue so you wouldn't miss it. I wasn't trying to short circuit but just make sure you saw the comment and replied to it. I can be more explicit the next time I ask for feedback.

• benschac added a child revision: D3493: [web] [refactor] move props to single destructure in top of chat thread list item menu component.Mar 23 2022, 6:43 AM

I wanted to make sure it was in your queue so you wouldn't miss it.

Wouldn't miss what? If you just want me to see the diff, you should add me to the CC list (I'm on the CC list of every diff already, though). If you wanted me to review the diff, then you should clarify the reason for short-circuiting review process.

My best is that you simply misinterpreted my earlier comment?

• benschac mentioned this in D3499: [web] [feat] add mark as unread styles for selected and hovered states.Mar 23 2022, 8:18 AM

Closed by commit rCOMM216d3994bfb1: [web] [fix] style mark as unread (authored by • benschac). · Explain WhyMar 24 2022, 12:30 PM

This revision was automatically updated to reflect the committed changes.

• benschac added a commit: rCOMM216d3994bfb1: [web] [fix] style mark as unread.

• benschac mentioned this in rCOMMc2358a3f9af8: [web] [feat] add mark as unread styles for selected and hovered states.