[web] Fix spacing regression in `MessageList` component
ClosedPublic
Actions

Authored by atul on Aug 9 2022, 8:58 AM.

Details

Reviewers

• abosh
varun
tomek
• jacek
ashoat

Commits

rCOMM5a762fcf5d1f: [web] Fix spacing regression in `MessageList` component

Summary

Context: https://linear.app/comm/issue/ENG-761/spacing-regression-between-conversations-in-messagelist

Previous approach: https://phab.comm.dev/D3537

There was a regression with the MessageList component where the spacing between the conversationHeader (AKA timestamp) and messages was removed.

Went through the git history of chat-message-list.css and checked to see if any commits changed the div.conversationHeader CSS selector and found D3067 to be relevant. Specifically, the diff removed the padding property (I believe unintentionally).

I reintroduced the padding property, but set vertical padding to 6px instead of 7px to maintain the relationship with font-size (which went from 14px to 12px).

Test Plan

Things look as expected:
Before:

Screen Shot 2022-08-09 at 11.59.15 AM.png (650×2 px, 380 KB)

After:

Screen Shot 2022-08-09 at 11.58.59 AM.png (650×2 px, 164 KB)

Messed around with the web app and looked around the code and couldn't find any side effects.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Aug 9 2022, 8:58 AM

Herald added a subscriber: • adrian. · View Herald TranscriptAug 9 2022, 8:58 AM

Adding @ashoat as reviewer since he created the issue and has context from previous attempts to fix regression

atul edited the summary of this revision. (Show Details)Aug 9 2022, 9:03 AM

Harbormaster completed remote builds in B11242: Diff 15476.Aug 9 2022, 9:11 AM

atul requested review of this revision.Aug 9 2022, 9:11 AM

This looks better!!

In D3537 I was pushing for a thorough analysis of spacing changes, but that's pretty expensive. Let's land this, but we should probably wait on closing the Linear task until we can play around with things on prod. "Play around with things on prod" can hopefully serve as an alternative to a thorough analysis.

This revision is now accepted and ready to land.Aug 9 2022, 9:42 AM

• abosh accepted this revision.Aug 9 2022, 10:05 AM

Let's land this, but we should probably wait on closing the Linear task until we can play around with things on prod.

Sounds good, I'll land but defer closing the Linear task (could also close now and re-open if the issue persists, but I guess it keeps it top of mind if it remains open).

Closed by commit rCOMM5a762fcf5d1f: [web] Fix spacing regression in `MessageList` component (authored by atul). · Explain WhyAug 9 2022, 10:33 AM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMM5a762fcf5d1f: [web] Fix spacing regression in `MessageList` component.

atul mentioned this in D3537: [web] [fix] ENG-761 add padding between timestamp and message.Aug 9 2022, 10:38 AM