Paths

Table of Contentst

[web] [fix] ENG-766 sidebar sidebar height chopped off + font size
ClosedPublic
Actions

Authored by • benschac on Mar 16 2022, 11:45 AM.

Details

Reviewers

atul
tomek
• jacek

Commits

rCOMM1d4de8a87620: [web] [fix] ENG-766 sidebar sidebar height chopped off + font size

Summary

Fix sidebar height chopping off per the issue. https://linear.app/comm/issue/ENG-766/sidebar-height-was-chopped-in-order-to-have-a-continuous-line
https://linear.app/comm/issue/ENG-766/sidebar-height-was-chopped-in-order-to-have-a-continuous-line

https://phabricator.ashoat.com/file/data/5fdirsmbi6qpkmd3im55/PHID-FILE-nvwfipkwd6aamahkzjdr/Image_2022-03-18_at_10.08.03_AM.jpg
Note: This diff introduces a regression in sidebars where the arrows aren't continuous. This issue will be fixed in a future diff in the stack.

Image 2022-03-16 at 2.47.05 PM.jpg (766×1 px, 68 KB)

Image 2022-03-16 at 2.47.30 PM.jpg (690×1 px, 85 KB)

Follow up on initial post:

Image 2022-04-13 at 2.21.43 PM.jpg (1×816 px, 75 KB)

Is the updated sidebar without chopped height. (this is a screenshot of the end of the stack). You'll notice an ever so thinner extender in the second thread. That's something I'll address moving forward either:

first try: using atul's SVG (https://blob.sh/atul/long_arrow.svg) with just an image tag like we do here: https://github.com/CommE2E/comm/blob/master/landing/read-docs-btn.react.js#L16
second: Following up with @palys-swm comment here: D3539 -- 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.

I know it's a subtle visual bug, but it will unblock my stack, and straighten out a lot of issues with the chat thread list items.

Test Plan

check figma, make sure it looks okay: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A79583

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• benschac created this revision.Mar 16 2022, 11:45 AM

Herald added subscribers: • karol, • adrian, tomek, ashoat. · View Herald TranscriptMar 16 2022, 11:45 AM

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

Harbormaster completed remote builds in B7412: Diff 10435.Mar 16 2022, 11:50 AM

• benschac requested review of this revision.Mar 16 2022, 11:50 AM

Note, I'm aligining the menu button (three dot vertical) in the next diff.

web/chat/chat-thread-list.css
130 ↗	(On Diff #10435)	The font was the wrong size. This could have been another diff. I figured it should be here since we're updating the sidebar and the font-size is going to effect how it looks visually.

• benschac added a child revision: D3441: [web] [fix] align unread buttons.Mar 16 2022, 12:28 PM

• benschac edited the summary of this revision. (Show Details)Mar 16 2022, 2:47 PM

atul added reviewers: tomek, • jacek.Mar 16 2022, 3:12 PM

The screenshot matches the design from https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A79456 - when a message has failed. When a message is successful, we display the message below the sidebar https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A79202. Could you share a screenshot of that scenario? Also, it would be great to see how it looks like when there are more sidebars.

Accepting, with the assumption that it looks correct when the message has not failed and when there are more sidebars.

This revision is now accepted and ready to land.Mar 17 2022, 10:49 AM

In D3440#93771, @palys-swm wrote:

The screenshot matches the design from https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A79456 - when a message has failed. When a message is successful, we display the message below the sidebar https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A79202. Could you share a screenshot of that scenario? Also, it would be great to see how it looks like when there are more sidebars.

Accepting, with the assumption that it looks correct when the message has not failed and when there are more sidebars.

More sidebars

Image 2022-03-18 at 10.08.03 AM.jpg (836×1 px, 65 KB)

needs to be adjusted more

we display the message below the sidebar

I didn't add that either to the thread functionality. This diff was only to adjust the height of the thread. Thanks for calling this out. I'll update this diff accordingly.

Not sure as to the details here, but we've previously discussed that the line connecting the sidebars should be continuous. At some point @benschac landed a diff that hacked the height in order to result in a continuous line. The height hack should be resolved in ENG-766.

We should not land this diff if it results in a regression on the continuity of the line.

tomek requested changes to this revision.Mar 21 2022, 2:17 AM

This revision now requires changes to proceed.Mar 21 2022, 2:17 AM

In D3440#94126, @ashoat wrote:

Not sure as to the details here, but we've previously discussed that the line connecting the sidebars should be continuous. At some point @benschac landed a diff that hacked the height in order to result in a continuous line. The height hack should be resolved in ENG-766.

Yes, that work still needs to be done. This is just getting the correct sidebar height. I wasn't going to land with a regression. Additionally, I could have put that in the description so you would have known.

We should not land this diff if it results in a regression on the continuity of the line.

Agreed

• benschac edited the summary of this revision. (Show Details)Mar 21 2022, 7:40 AM

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

• benschac mentioned this in D3480: [web] [fix] thread list size and paddings are incorrect.Mar 24 2022, 7:19 AM

• benschac retitled this revision from [web] [fix] sidebar ENG-766 sidebar height chopped off + font size to [web] [fix] ENG-766 sidebar sidebar height chopped off + font size.Mar 29 2022, 8:38 AM

rebase

Harbormaster completed remote builds in B8167: Diff 11432.Apr 13 2022, 10:37 AM

• benschac edited the summary of this revision. (Show Details)Apr 13 2022, 11:31 AM

tomek accepted this revision.Apr 14 2022, 1:08 AM

This revision is now accepted and ready to land.Apr 14 2022, 1:08 AM

Closed by commit rCOMM1d4de8a87620: [web] [fix] ENG-766 sidebar sidebar height chopped off + font size (authored by • benschac). · Explain WhyApr 19 2022, 9:49 AM

This revision was automatically updated to reflect the committed changes.

• benschac added a commit: rCOMM1d4de8a87620: [web] [fix] ENG-766 sidebar sidebar height chopped off + font size.