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.
Differential D3480
[web] [fix] thread list size and paddings are incorrect • benschac on Mar 21 2022, 12:36 PM. Authored by Tags None Referenced Files
Details 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. check figma, make sure screen shots are close.
Diff Detail
Event TimelineComment Actions 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) Comment Actions 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.
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.
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.
This comment was removed by ashoat. Comment Actions Thanks for sharing those screenshots with the horizontal lines – it makes it clear that this diff matches the designs in Figma.
It's important for virtualized lists, but since this list isn't virtualized right now it's probably not a big deal.
Should this diff also be blocked until ENG-911 is resolved? Comment Actions 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. |