Details

Reviewers

atul
ashoat
kamil

Commits

rCOMM5448df76c019: [web] Implement darker thread color tint to blockquote container

Summary

Linear issue: ENG-1712. Restyling the quotes by layering the blockquote with a low opacity black background to create the darker tint effect

Next Steps:

Update non-viewer blockquote message container color to be the base thread color

Test Plan

Please view the attached screenshots to see the target design in the figma document and the before and after of the changes I implemented

Figma document: Figma.

Screen Shot 2022-09-03 at 3.22.23 PM.png (1×1 px, 92 KB)

Before:

Screen Shot 2022-09-07 at 2.02.57 PM.png (586×1 px, 74 KB)

After:

Screen Shot 2022-09-20 at 10.22.04 PM.png (554×1 px, 58 KB)

Screen Shot 2022-09-20 at 10.24.02 PM.png (552×1 px, 83 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Harbormaster completed remote builds in B11940: Diff 16391.Sep 7 2022, 12:10 AM

Harbormaster completed remote builds in B11941: Diff 16392.

Are you running into the following when you try to run things locally?

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/atul/comm/keyserver/dist/web/markdown/markdown.css' imported from /Users/atul/comm/keyserver/dist/web/markdown/rules.react.js

I'm not able to patch in the changes and test it out. Assuming it's not just an issue on my end, we can schedule something to walk through it?

This revision now requires changes to proceed.Sep 7 2022, 9:27 PM

atul retitled this revision from [WEB] implemnted darker thread color tint to blockquote container to [web] Implement darker thread color tint to blockquote container.Sep 12 2022, 10:30 AM

Glad I caught this diff before it got accepted. It might be best for me to do a second-pass review of all Markdown diffs until @atul is able to get up to speed with the codebase

web/chat/text-message.react.js
77	Not a fan of the special-casing of `threadInfo.color` here. Can't this be moved to do something similar to this?
web/markdown/markdown.react.js
42–50	Is this the right place for this logic? I'd love to see if we can move it to `rules.react.js`

changed threadColor logic to go through rules.react.js

fixed weird git tracking issue

put back useDarkStyle logic in markdown.react.js

Harbormaster completed remote builds in B12235: Diff 16787.Sep 18 2022, 5:54 AM

ginsu edited the summary of this revision. (Show Details)Sep 18 2022, 5:55 AM

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F172911: Screen Shot 2022-09-18 at 9.52.44 PM.png. (Show Details)

Harbormaster completed remote builds in B12236: Diff 16788.Sep 18 2022, 5:57 AM

Harbormaster completed remote builds in B12237: Diff 16789.Sep 18 2022, 6:01 AM

ginsu edited the summary of this revision. (Show Details)Sep 18 2022, 6:38 AM

ashoat requested changes to this revision.Sep 18 2022, 1:48 PM

ashoat added a subscriber: kamil.

ashoat added inline comments.

web/markdown/rules.react.js
87 ↗	(On Diff #16789)	Pretty sure you're reverting @kamil's recent changes in D5033 here...
106 ↗	(On Diff #16789)	Can `tinycolor` really parse this? If so, how does it do it? Can you link to docs?
190 ↗	(On Diff #16789)	Reverse this condition, see here
193 ↗	(On Diff #16789)	We should avoid implicit `return`s like this, and explicitly `return undefined` so it's clear to the reader

This revision now requires changes to proceed.Sep 18 2022, 1:48 PM

Would be good for @kamil to take a look since he touched this code recently

Hi @ginsu.
Not sure but looks like you unintentionally badly resolved conflicts after rebasing with master because this update: D4999#151387 has different base commit - this caused those changes you introduced.

web/markdown/rules.react.js
87 ↗	(On Diff #16789)	Pretty sure you're reverting @kamil's recent changes in D5033 here... That is correct. You should use `SharedMarkdown.matchBlockQuote(SharedMarkdown.blockQuoteRegex)` as matching function which basically does the same but adds nested quotation limit.
88–97 ↗	(On Diff #16789)	Here should be different parsing function: `SharedMarkdown.parseBlockQuote`

resovled ashoat's and kamil's comments, my bad on unintentionally reverting kamils changes. Also good shout on tinycolor not parsing CSS custom properties, I made some changes to fix that

Harbormaster completed remote builds in B12268: Diff 16830.Sep 19 2022, 6:27 AM

ginsu added a child revision: D5178: [web] Changed non-viewer blockquote message container color to be the base thread color.Sep 19 2022, 7:06 AM

ashoat requested changes to this revision.Sep 19 2022, 10:20 AM

ashoat added inline comments.

web/markdown/rules.react.js
104 ↗	(On Diff #16830)	What happens if you pass `undefined` to `tinycolor`? How does this get handled in the SSR (Server-Side Rendering) context? Seems like it may lead to errors due to inconsistent results on server vs. client My instinct is that we might have to find another way to pass this info to `web`... pulling it from CSS might not work in our case. Some ideas: We could try using the first option on this tutorial, and then to update `web/theme.css` to reference those variables We could just copy-paste the CSS variable value into JS, and add code comments indicating that the two values should be kept in sync. Not a great solution, but an acceptable fallback if 1 doesn't work

This revision now requires changes to proceed.Sep 19 2022, 10:20 AM

Separately, how confident are you that we absolutely NEED to pass in the threadColor here? Is there no way to use CSS with opacity or something to avoid needing the threadColor as an input? My suspicion is that there is a way more simple solution here that you haven't explored...

reimplemented solution by removing threadColor logic and creating an opacity layer

removed extra blank line

ginsu edited the test plan for this revision. (Show Details)Sep 20 2022, 3:56 AM

ginsu attached a referenced file: F175276: Screen Shot 2022-09-20 at 7.56.15 PM.png. (Show Details)

cleaned up border-left css

Harbormaster completed remote builds in B12304: Diff 16892.Sep 20 2022, 4:00 AM

Harbormaster completed remote builds in B12305: Diff 16893.Sep 20 2022, 4:03 AM

ginsu edited the test plan for this revision. (Show Details)Sep 20 2022, 4:03 AM

ginsu attached a referenced file: F175318: Screen Shot 2022-09-20 at 8.03.19 PM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)

ginsu edited the summary of this revision. (Show Details)Sep 20 2022, 4:06 AM

Harbormaster completed remote builds in B12306: Diff 16894.Sep 20 2022, 4:09 AM

Looks WAY cleaner!!

Nice!

As an aside, I really liked the design with the drop shadow you shared in the Linear issue (https://linear.app/comm/issue/ENG-1712/restyle-quotes-in-messages-to-match-new-designs):

I thought it provided more "contrast" between quotes when there are multiple levels of nesting, and just generally looked a lot better.

CC @ashoat for thoughts. I realize the design was already determined in the Linear issue, but wanted to re-open discussion because I strongly prefer the design with drop shadows.

Ah yeah, I got confused and thought there were only 3 options... I should've specified my fav using the title. I think "Left Border with chat theme and drop shadow" is the best in that list

If it's easy to implement with CSS I think we should give it a go!

I'll take a look into it right now!

added box-shadow property

ginsu edited the test plan for this revision. (Show Details)Sep 20 2022, 6:26 AM

ginsu attached a referenced file: F175567: Screen Shot 2022-09-20 at 10.22.04 PM.png. (Show Details)

ginsu attached a referenced file: F175569: Screen Shot 2022-09-20 at 10.24.02 PM.png. (Show Details)

nice