Page MenuHomePhabricator

[web] Introduce thread top bar
ClosedPublic

Authored by jacek on Feb 9 2022, 5:38 AM.
Tags
None
Referenced Files
F3243453: D3147.id9652.diff
Thu, Nov 14, 8:49 AM
F3243452: D3147.id9650.diff
Thu, Nov 14, 8:49 AM
F3243451: D3147.id9477.diff
Thu, Nov 14, 8:49 AM
Unknown Object (File)
Wed, Nov 13, 12:30 PM
Unknown Object (File)
Sun, Nov 10, 11:57 PM
Unknown Object (File)
Sun, Nov 10, 11:57 PM
Unknown Object (File)
Sun, Nov 10, 11:57 PM
Unknown Object (File)
Sun, Nov 10, 11:57 PM

Details

Summary

Introduced thread top bar component with thread name, ancestors (in the following diff) and settings menu on the right.

Screenshot_Google Chrome_2022-02-09_144706.png (176×709 px, 13 KB)

Test Plan

Compared result with design in figma and confirmed, that it is displayed correctly for all threads.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

benschac added inline comments.
web/chat/chat-thread-top-bar.react.js
14 ↗(On Diff #9443)

https://phabricator.ashoat.com/D3148 seems very similar to this logic/color implementation. Could be worth making it a hook?

16 ↗(On Diff #9443)

nit: been using background for the most part. I _feel_ like it's a bit easier to notice. background v. color.

This revision is now accepted and ready to land.Feb 9 2022, 6:43 AM
This revision now requires review to proceed.Feb 9 2022, 6:43 AM
tomek requested changes to this revision.Feb 9 2022, 7:31 AM
tomek added inline comments.
web/chat/chat-thread-top-bar.react.js
1–2 ↗(On Diff #9443)

We have a convention to keep a newline after flow annotation

12 ↗(On Diff #9443)

We should try to have component and file names consistent, so either change function name to ChatThreadTopBar or change file name to thread-top-bar.

This revision now requires changes to proceed.Feb 9 2022, 7:31 AM
jacek edited the summary of this revision. (Show Details)

Renamed file, replaced backgroundColor with background, added newline after flow annotation

Wow, this looks great!!

This revision is now accepted and ready to land.Feb 9 2022, 9:56 PM
web/chat/thread-top-bar.react.js
29 ↗(On Diff #9477)

nit: can this be a p. Sorry I missed this last time

Replaced div with p in title element.

web/chat/thread-top-bar.react.js
29 ↗(On Diff #9477)

@benschac please do not leave comments requesting changes without actually hitting the "Request Changes" button. I know you probably feel that requesting changes is intense, but by leaving a comment like this you aren't putting this diff in the author's queue. Please be thoughtful about diff queues going forward

web/chat/thread-top-bar.react.js
29 ↗(On Diff #9477)

Hmm actually in this case it's already in @def-au1t's queue to land. Maybe this isn't as big of a deal as I originally thought

This revision was landed with ongoing or failed builds.Feb 15 2022, 3:39 AM
This revision was automatically updated to reflect the committed changes.