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.
Details
Details
check figma, make sure screen shots are close.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment 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.
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
- Production: 67px height
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?
- 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).
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.
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?
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.