Page MenuHomePhabricator

[web] [feat] [ENG-766] toggle longer arrow for multiple thread sidebars in chat thread list
ClosedPublic

Authored by benschac on Mar 29 2022, 8:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 4:35 PM
Unknown Object (File)
Wed, Nov 27, 2:25 PM
Unknown Object (File)
Wed, Nov 27, 12:51 PM
Unknown Object (File)
Sat, Nov 9, 10:45 AM
Unknown Object (File)
Sat, Nov 9, 10:43 AM
Unknown Object (File)
Sat, Nov 9, 9:53 AM
Unknown Object (File)
Sat, Nov 9, 8:58 AM
Unknown Object (File)
Sat, Nov 9, 7:13 AM

Details

Summary

adds an extender to remove gap between lines. It's not perfect. I need to figure out a way to make it blur so it matches the svg more closely. If you zoom in you can see it but, it's very very slight. This diff will unblock the rest of the stack

before:

Image 2022-03-29 at 11.27.40 AM.jpg (712×824 px, 61 KB)

after:
Image 2022-03-29 at 11.32.59 AM.jpg (388×828 px, 28 KB)

Test Plan

add many threads to a conversation, arrows should connect. https://linear.app/comm/issue/ENG-941/arrow-extenders-should-match-blur-of-svg

Diff Detail

Repository
rCOMM Comm
Branch
fix-side-bar
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

benschac edited the test plan for this revision. (Show Details)
benschac edited the summary of this revision. (Show Details)
benschac edited the summary of this revision. (Show Details)
benschac retitled this revision from [web] [feat] add arrow extender to thread sidebar to [web] [feat] ENG-766 add arrow extender to thread sidebar.Mar 29 2022, 8:37 AM
web/chat/chat-thread-list.css
50 ↗(On Diff #10773)

Why are there two height rules for the same element? The height: 10px can be deleted as the height: 8px should override any styling caused by the prior height rule.

remove height

web/chat/chat-thread-list.css
50 ↗(On Diff #10773)

it was an oversight. thanks for catching it.

web/chat/sidebar-item.react.js
31

I'd prefer to avoid ternary that returns JSX like this. I realize the rules around when to use ternary syntax are pretty complex, so I wrote up a note with everything spelled out

Going to close this in favor of a longer arrow.

(solution where if the conversation has many threads, a longer arrow will be rendered to show a continuous line between threads)

still can use this diff. Just need to swap out the arrow.

benschac retitled this revision from [web] [feat] ENG-766 add arrow extender to thread sidebar to [web] [feat] [ENG-766] add arrow extender to thread sidebar.Apr 5 2022, 8:16 AM
web/chat/sidebar-item.react.js
31

To follow up on this -- when you say "avoid ternary that returns JSX," does that include the logical && syntax? From the React Docs:

image.png (440×1 px, 64 KB)

The code could then be revised to:

{extendArrow && <div className={css.threadArrowExtender} />}

If not, however, then above the return statement it would just be

let arrowExtender;
if (extendArrow) {
  arrowExtender = <div className={css.threadArrowExtender} />;
} else {
  arrowExtender = null;
}
web/chat/sidebar-item.react.js
31

Good question – yeah the intention is to include that sort of syntax. The rule is about surfacing conditionals clearly to the reader

benschac retitled this revision from [web] [feat] [ENG-766] add arrow extender to thread sidebar to [web] [feat] [ENG-766] toggle longer arrow for multiple thread sidebars in chat thread list.Apr 8 2022, 7:24 AM

It looks ok as a workaround. Have you tested how it behaves if we zoom the page or resize it?

For me it looks like the best way to make it consistent with the rest of an arrow is to draw this whole shape via css. What do you think?

web/chat/chat-thread-list-sidebar.react.js
15

An item is never a multiple sidebar. How about isSubsequentItem?

This revision is now accepted and ready to land.Apr 13 2022, 4:56 AM
In D3539#102420, @palys-swm wrote:

It looks ok as a workaround. Have you tested how it behaves if we zoom the page or resize it?

For me it looks like the best way to make it consistent with the rest of an arrow is to draw this whole shape via css. What do you think?

These questions were both answered in https://phabricator.ashoat.com/D3782

Following up here to make sure I close the loop.