Introduce thread ancestors list in top thread bar and keyserver owner. I tried to exactly match Figma design.
Right now, it doesn't show names of threads between source and current thread, but it can be easily introduced if there is a need.
Details
Details
Tested the component on threads with different count of ancestors to confirm it is displayed correctly.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jacek/thread-top-bar
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
small questions / nits. Looks good!
| web/chat/chat-thread-ancestors.react.js | ||
|---|---|---|
| 18 ↗ | (On Diff #9444) | nit: const { color: threadColor } = threadInfo; |
| 23 ↗ | (On Diff #9444) | had no clue this existed. Nice! |
| 38 ↗ | (On Diff #9444) | maybe call this keyserverOwnerUsername? |
| 39 ↗ | (On Diff #9444) | I don't think we need to explicitly type member I could be wrong. |
| web/chat/chat-thread-ancestors.css | ||
|---|---|---|
| 13 ↗ | (On Diff #9444) | This should be defined for all the divs, globally. Nothing to change in this diff, but we should probably address this during the redesign. @benschac what do you think? |
| 14 ↗ | (On Diff #9444) | I don't think this is a good idea to set a fixed height. The height should be computed based on font size and the component should be positioned by its parent. @benschac am I right? |
| web/chat/chat-thread-ancestors.react.js | ||
| 1 ↗ | (On Diff #9444) | |
| 19 ↗ | (On Diff #9444) | It doesn't matter here, but this value will be computed on every render. It should be computed only when threadColor changes, so we could do that inside the memo, which is recomputed when threadColor changes. But that's just a nit. |
| 20–32 ↗ | (On Diff #9444) | We destructured threadColor from threadInfo, so we should use it here |
| 43 ↗ | (On Diff #9444) | It's better to have an explicit return undefined |
| 44 ↗ | (On Diff #9444) | We depend only on community.members |
| 46 ↗ | (On Diff #9444) | It doesn't hurt to memoize these, but I'm not sure if it's important. It looks like we need to recompute only rarely, so maybe it is a good idea. |
| 94 ↗ | (On Diff #9444) | We're not interested here in the whole community and only need to know if community === threadInfo so we should compute that before memo and use as a dependency. Then we can replace threadInfo with threadInfo.uiName as a dep. |
Comment Actions
Some nits inline
| web/chat/chat-thread-ancestors.react.js | ||
|---|---|---|
| 21 ↗ | (On Diff #9478) | This isn't just the background color, it also includes the text color. So maybe it should just be named threadColorStyle |
| 30 ↗ | (On Diff #9478) | And then can be called fullStructureButtonColorStyle or something |
| 75 ↗ | (On Diff #9478) | Should we use a Unicode ellipsis here instead? |
| web/chat/chat-thread-ancestors.css | ||
|---|---|---|
| 13 ↗ | (On Diff #9444) | yes 100%. We have a reset on /landing which applies box-sizing and a bunch of other defaults. |
| 14 ↗ | (On Diff #9444) | yeah, I missed this. hight should be font-size. |
| 20 ↗ | (On Diff #9444) | Not for this diff. But we should go back and delete button styles now that we have a component. |
| web/chat/chat-thread-ancestors.css | ||
|---|---|---|
| 20 ↗ | (On Diff #9444) | @benschac: You've left a couple of "not for this diff" comments... these are bound to get lost. As the reviewer, if there's something you'd like the author to follow up on, you should request their either create a follow-up diff or a Linear task before landing. And it can be helpful to describe exactly what you're requesting in more detail... in this case and the other case you left a "not for this diff" comment today, the comment felt too short, and I doubt it conveyed enough info to the author for them to know what to do |
| web/chat/chat-thread-ancestors.css | ||
|---|---|---|
| 20 ↗ | (On Diff #9444) | https://linear.app/comm/issue/ENG-741/clean-up-button-styles-in-the-app Added to the backlog. |
This comment was removed by ashoat.