Page MenuHomePhabricator

[web] [darkmode] unread button
ClosedPublic

Authored by benschac on Feb 11 2022, 11:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 12:31 PM
Unknown Object (File)
Sun, Nov 10, 11:58 PM
Unknown Object (File)
Sun, Nov 10, 11:58 PM
Unknown Object (File)
Sun, Nov 10, 11:58 PM
Unknown Object (File)
Sun, Nov 10, 11:58 PM
Unknown Object (File)
Sun, Nov 10, 11:11 PM
Unknown Object (File)
Tue, Nov 5, 12:50 AM
Unknown Object (File)
Sat, Oct 26, 8:08 PM

Details

Summary

theme unread button, no design in web. Generally just theme'ed with our color palette.

Image 2022-02-11 at 2.00.25 PM.jpg (312×440 px, 17 KB)

Adding ashoat since Atul is OOO today.

Test Plan

should work as before.

Diff Detail

Repository
rCOMM Comm
Branch
delete (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

benschac edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Feb 13 2022, 8:57 PM

Is @atul still OOO this week? Why was I added here but not to D3180?

web/chat/chat-thread-list-item-menu.react.js
78 ↗(On Diff #9576)

Since you're not using menuContentVisible anymore, you should delete the associated CSS selector in chat-thread-list.css

This revision now requires changes to proceed.Feb 13 2022, 8:57 PM
In D3181#84766, @ashoat wrote:

Is @atul still OOO this week? Why was I added here but not to D3180?

  1. Not that I know of.
  2. Thought there was more urgency here because unread is a important visual indicator.

Looks good. Outside the scope of this diff, but:

  1. I think we should find and use a vertical ellipsis icon without a small hole in each circle.
  2. We should give the Mark as read button a drop shadow so there's some depth.
web/chat/chat-thread-list-item-menu.react.js
78 ↗(On Diff #9605)

Can just do let btn;

92 ↗(On Diff #9605)

Does this need to be in a <div>?

Could we just do {btn}?

web/components/button.react.js
11 ↗(On Diff #9605)

Instead of unread, which might not be clear to the reader.. maybe toggle_read or toggle_read_status or something?

@benschac, can you make sure to create follow-ups for the tasks @atul identified before landing?

This revision is now accepted and ready to land.Feb 14 2022, 9:33 PM
In D3181#85170, @atul wrote:

Looks good. Outside the scope of this diff, but:

  1. I think we should find and use a vertical ellipsis icon without a small hole in each circle.
  2. We should give the Mark as read button a drop shadow so there's some depth.
  1. That's quite literally what's in Figma. We'd need to find a different icon pack or get someone to make an icon. https://linear.app/comm/issue/ENG-770/fix-vertical-ellipsis-icon-so-it-doesnt-have-a-hole-in-the-middle-of
  2. https://linear.app/comm/issue/ENG-769/mark-as-read-button-a-drop-shadow-so-theres-some-depth

address diff comments

web/chat/chat-thread-list-item-menu.react.js
92 ↗(On Diff #9605)

Image 2022-02-15 at 9.41.56 AM.jpg (384×538 px, 23 KB)

yes. I didn't want to refactor the code even more.

This revision was landed with ongoing or failed builds.Feb 15 2022, 6:46 AM
This revision was automatically updated to reflect the committed changes.
In D3181#85170, @atul wrote:

Looks good. Outside the scope of this diff, but:

  1. I think we should find and use a vertical ellipsis icon without a small hole in each circle.
  2. We should give the Mark as read button a drop shadow so there's some depth.
  1. That's quite literally what's in Figma. We'd need to find a different icon pack or get someone to make an icon. https://linear.app/comm/issue/ENG-770/fix-vertical-ellipsis-icon-so-it-doesnt-have-a-hole-in-the-middle-of
  2. https://linear.app/comm/issue/ENG-769/mark-as-read-button-a-drop-shadow-so-theres-some-depth