Page MenuHomePhabricator

[web] [fix] thread list size and paddings are incorrect
ClosedPublic

Authored by benschac on Mar 21 2022, 12:36 PM.
Tags
None
Referenced Files
F3393111: D3480.id10895.diff
Sat, Nov 30, 11:34 AM
Unknown Object (File)
Wed, Nov 27, 12:29 PM
Unknown Object (File)
Mon, Nov 11, 6:51 AM
Unknown Object (File)
Mon, Nov 11, 6:51 AM
Unknown Object (File)
Mon, Nov 11, 6:51 AM
Unknown Object (File)
Sat, Nov 9, 1:04 PM
Unknown Object (File)
Sat, Nov 9, 12:16 PM
Unknown Object (File)
Sat, Nov 9, 8:58 AM

Details

Summary

fix padding sizes to reflect figma more closely https://www.figma.com/file/RROVwCKAeFaO7tG64blzlx/Comm-Web-Design-System?node-id=1%3A17

still not 100% but a lot closer. The rest of the visual issues will be solved in future diffs.

Image 2022-03-21 at 3.38.26 PM.jpg (176×822 px, 11 KB)

Test Plan

check figma, make sure screen shots are close.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul accepted this revision.EditedMar 22 2022, 2:11 PM

Certainly looks closer to what's on Figma, but it looks like there's too much vertical space between elements? It might be that the thread title is lowercase so it's not taking up the same vertical space as the examples?


Did you do any testing to make sure that this change doesn't have any side effects/consequences? We should make sure that this diff only changes the position of things and doesn't increase the overall height of the thread list "cell."

It looks like we explicitly set a fixed height of 32px for div.threadListSidebar. Correct me if I'm wrong, but I'm it doesn't look like we're doing that for each thread list "cell"?

Worried that this might cause a regression with, for example, the sidebar arrows that iirc are fixed size and absolutely positioned?


(Adding @ashoat as blocking reviewer to take a look)

This revision is now accepted and ready to land.Mar 22 2022, 2:11 PM
This revision now requires review to proceed.Mar 22 2022, 2:12 PM
ashoat requested changes to this revision.Mar 22 2022, 10:01 PM

Requesting changes for @atul's comment above. Agree on each point

This revision now requires changes to proceed.Mar 22 2022, 10:01 PM
In D3480#94936, @atul wrote:

Certainly looks closer to what's on Figma, but it looks like there's too much vertical space between elements? It might be that the thread title is lowercase so it's not taking up the same vertical space as the examples?

Image 2022-03-24 at 9.39.40 AM.jpg (984×1 px, 547 KB)

I am assuming the elements you're asking about are text elements and not the chat thread list items? Looking at the figma and localhost they seem pretty close. If this seems incorrect let's hop on a tuple and reduce churn.


Did you do any testing to make sure that this change doesn't have any side effects/consequences? We should make sure that this diff only changes the position of things and doesn't increase the overall height of the thread list "cell."

We do want to change the height of the thread list cell! My intention was to make these "cells" larger to match figma more closely. I did the normal resize and general use testing before putting up the diff.

  • Figma: 84px height
    Image 2022-03-24 at 9.45.04 AM.jpg (650×966 px, 89 KB)
  • Production: 67px height
    Image 2022-03-24 at 9.46.05 AM.jpg (234×894 px, 40 KB)

It looks like we explicitly set a fixed height of 32px for div.threadListSidebar. Correct me if I'm wrong, but I'm it doesn't look like we're doing that for each thread list "cell"?

Correct: I was looking at the figma and trying to get the positioning of the inner elements correct so that it would be 84px height rather than setting a height. The 32px div.threadListSidebar could be done the same way. I was set the height thinking it would be easier to absolutely position the arrow. I can update this diff with an absolute height if your think that's a better approach.

Worried that this might cause a regression with, for example, the sidebar arrows that iirc are fixed size and absolutely positioned?

This revision is now accepted and ready to land.Mar 24 2022, 7:19 AM

back to atul for review.

This comment was removed by ashoat.

Thanks for sharing those screenshots with the horizontal lines – it makes it clear that this diff matches the designs in Figma.

I can update this diff with an absolute height if your think that's a better approach.

It's important for virtualized lists, but since this list isn't virtualized right now it's probably not a big deal.

sidebar arrows: that regression was introduced in D3440 because the arrows aren't long enough and will hopefully be resolved when https://linear.app/comm/issue/ENG-911/extend-arrow-svg-height-in-thread-sidebar-design is completed. D3440 should be blocked until ENG-911 is resolved (and hopefully fixes the problem).

Should this diff also be blocked until ENG-911 is resolved?

This revision is now accepted and ready to land.Mar 24 2022, 5:50 PM
In D3480#95747, @ashoat wrote:

sidebar arrows: that regression was introduced in D3440 because the arrows aren't long enough and will hopefully be resolved when https://linear.app/comm/issue/ENG-911/extend-arrow-svg-height-in-thread-sidebar-design is completed. D3440 should be blocked until ENG-911 is resolved (and hopefully fixes the problem).

Should this diff also be blocked until ENG-911 is resolved?

No, this is the chat thread list item. The arrow is absolutely positioned to the thread list item. Because the chat thread list item is a little bigger, the arrow will look different but closer to the actual design.

ENG-911 addresses the arrows being a continuous straight line.