Details

Reviewers

atul
ashoat
kamil
rohan

Commits

rCOMMed5ec298ee30: [native] Implement darker color tint to block quote container

Summary

Linear issue: ENG-1712.

The block quote containers now match the shape and color shown in the Figma document

Unfortunately, by developing the darker tint with the opacity layer, the shadow effect we have on the web won't be possible due to some limitations with how React Native handles shadows. React Native does not have a box-shadow style property, and the React Native shadow properties requires a colored background of some sort.

For reference, this is what the blockquote looks like with the shadow effect with the opacity layer:

Screen Shot 2022-09-27 at 3.22.42 PM.png (1×1 px, 793 KB)

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-03 at 3.23.04 PM.png (1×760 px, 565 KB)

After:

Screen Shot 2022-09-27 at 1.24.38 PM.png (1×1 px, 680 KB)

Screen Shot 2022-09-27 at 4.04.54 PM.png (1×1 px, 600 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

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptSep 2 2022, 11:19 PM

ginsu edited the summary of this revision. (Show Details)Sep 2 2022, 11:24 PM

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

ginsu added reviewers: ashoat, • abosh.

ginsu edited reviewers, added: atul; removed: ashoat.

Harbormaster completed remote builds in B11869: Diff 16282.Sep 2 2022, 11:29 PM

ginsu requested review of this revision.Sep 2 2022, 11:29 PM

Hey Ginsu, looks like your files weren't attached.

It's because they're still "Restricted Files":

Phabricator can be tricky about this. Here's a doc explaining how to attach images on Phabricator.

This revision now requires changes to proceed.Sep 2 2022, 11:41 PM

• abosh attached a referenced file: F158119: image.png. (Show Details)Sep 2 2022, 11:42 PM

ginsu attached a referenced file: F158106: Screen Shot 2022-09-03 at 3.14.16 PM.png. (Show Details)Sep 2 2022, 11:45 PM

ginsu attached a referenced file: F158105: Screen Shot 2022-09-03 at 3.23.04 PM.png. (Show Details)

ginsu attached a referenced file: F158104: Screen Shot 2022-09-03 at 3.22.23 PM.png. (Show Details)

Cool, I can see them now! But your revision is still in a Needs Revision state. That means it's been removed from reviewers' queues on the Phabricator home screen (under Ready to Review). If you want reviewers to see this diff, you should submit the Request Review action (same place as Abandon Revision). Every time you update a diff or want reviewers to take a look at it again, submit this action. If you want to remove a diff from reviewers' queues because you plan to make changes, submit the Plan Changes action.

ginsu requested review of this revision.Sep 2 2022, 11:53 PM

Patched this diff in my local environment and it looks great! Left two pieces of feedback, but it should be good once those are addressed

native/chat/inner-text-message.react.js
97–111 ↗	(On Diff #16282)	Can we memoize the style object w/ `React.useMemo(...)` here
native/markdown/markdown.react.js
66–69 ↗	(On Diff #16282)	Since `threadColor` is optional, we should handle the case where it's `null/undefined`. If we don't explicitly decide on a default color in the `null/undefined` case we end up with `color` being set to black (`#000000`): We could maybe stick to the existing gray in that case? Something like: const color = threadColor ? tinycolor(threadColor).darken(20).toString() : "default_existing_gray_here";

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

Resovled Atul's comments

Harbormaster failed remote builds in B12031: Diff 16528!Sep 8 2022, 6:35 PM

Resolving atuls comments again because it did not seem to push correctly first time

Harbormaster completed remote builds in B12032: Diff 16529.Sep 8 2022, 9:05 PM

atul retitled this revision from ENG-1712 [native] implemnted darker color tint to block quote container to [native] Implement darker color tint to block quote container.Sep 12 2022, 10:30 AM

atul added a child revision: D5049: [native] Update non-viewer blockquote message container color to be base thread color.Sep 12 2022, 6:52 PM

atul attached a referenced file: F162525: 3aa9e6.png. (Show Details)Sep 13 2022, 11:51 AM

Looks really good for one level of quote... but doesn't look great when there are additional levels of nesting:

In the past we were handling this with

borderLeftColor: 'blockQuoteBorder',
borderLeftWidth: 5,

I couldn't find any designs on Figma that show how we should handle the multiple nested quote case..

One approach could be to further darken each level of quote to indicate the hierarchy. This probably wouldn't be great because you can only darken each color so much. (Though IIRC I might have read somewhere that we might want to limit the level of nested quotes in the future? CC @kamil)

Another approach might be to bring back the indented vertical bars on the left to indicate the depth. It might look something like this:

Simulator Screen Shot - iPhone 13 Pro - 2022-09-13 at 16.06.03.png (2×1 px, 608 KB)

Another approach might be adding some sort of border stroke or drop shadow to each quote to distinguish between them... this could be combined with either of the two previous ideas.

CC @ashoat to get his take before we proceed with any design decisions

native/markdown/markdown.react.js
70–73 ↗	(On Diff #16529)	Could we try something like this instead?

This revision now requires changes to proceed.Sep 13 2022, 1:09 PM

Similar to one of the suggested ideas but with a thin solid outline:

Simulator Screen Shot - iPhone 13 Pro - 2022-09-13 at 16.12.28.png (2×1 px, 581 KB)

One approach could be to further darken each level of quote to indicate the hierarchy. This probably wouldn't be great because you can only darken each color so much. (Though IIRC I might have read somewhere that we might want to limit the level of nested quotes in the future? CC @kamil)

We wanted to limit the level of quotation to 5 since more levels were causing problems on native. FYI I've just landed a diff with this: https://phab.comm.dev/D5033.

ashoat requested changes to this revision.Sep 15 2022, 1:00 PM

ashoat added inline comments.

native/chat/inner-text-message.react.js
120 ↗	(On Diff #16529)	Same feedback as here. `rules` should already have this bound in

removed all the threadColor logic and instead created darker tint through an opacity layer

Harbormaster failed remote builds in B12472: Diff 17119!Sep 27 2022, 10:57 AM

ginsu edited the summary of this revision. (Show Details)Sep 27 2022, 12:20 PM

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

ginsu attached a referenced file: F183777: Screen Shot 2022-09-27 at 1.24.38 PM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)Sep 27 2022, 12:26 PM

ginsu attached a referenced file: F183832: Screen Shot 2022-09-27 at 3.22.42 PM.png. (Show Details)

ginsu mentioned this in D5049: [native] Update non-viewer blockquote message container color to be base thread color.Sep 27 2022, 1:02 PM

ginsu edited the test plan for this revision. (Show Details)Sep 27 2022, 1:05 PM

ginsu attached a referenced file: F183902: Screen Shot 2022-09-27 at 4.04.54 PM.png. (Show Details)

atul added a reviewer: kamil.Sep 27 2022, 1:20 PM

Harbormaster completed remote builds in B12472: Diff 17119.Sep 27 2022, 1:32 PM

I left some comments but it'll be good if one of the senior reviewers take a look if my suggestions were correct.

native/chat/inner-text-message.react.js
94 ↗	(On Diff #17119)	It's more like `textColor` for me but not sure
94–105 ↗	(On Diff #17119)	Maybe it'll be better to merge those two into one `React.useMemo()`. Sth like: const markdownStyles = React.useMemo(() => { const textColor = { color: darkColor ? colors.dark.listForegroundLabel : colors.light.listForegroundLabel, }; return [styles.text, textColor]; }, [darkColor]);
native/markdown/rules.react.js
228–229 ↗	(On Diff #17119)	We changing something only on the first, outermost quote so maybe more clear will be relying only on the `boolean` value because there is no need to indicate each level. And then in line 239: `{ ...state, isNestedQuote: true }`
234–237 ↗	(On Diff #17119)	I think notation with the array is used more frequently in the codebase, other than that when we pass array react native takes the second argument as the right one while in object spreading styles merging is happening on javascript's object merging level.

Requesting changes so @kamil's feedback can be addressed

native/chat/inner-text-message.react.js
94 ↗	(On Diff #17119)	Agree that as of now the style object is essentially just handling the `textColor`, but think `Style` suffix to indicate that it's a style object vs a single property makes sense. It was also named `textStyle` before it was memoized, so think it's fine to leave as is.
94–105 ↗	(On Diff #17119)	I think this feedback makes sense
native/markdown/rules.react.js
228–229 ↗	(On Diff #17119)	I think this feedback makes sense (and leads to a cleaner/clearer solution)
234–237 ↗	(On Diff #17119)	I think this feedback makes sense

This revision now requires changes to proceed.Sep 28 2022, 7:37 AM

resolved kamils comments

atul added a reviewer: rohan.Sep 28 2022, 10:46 AM

Harbormaster completed remote builds in B12497: Diff 17152.Sep 28 2022, 10:46 AM

Looks good to me, adding @ashoat as blocking since he has much more context here

native/chat/inner-text-message.react.js
93–114	Since the changes to this file are no longer relevant to this diff, it would have been good to separate the memoization of `markdownStyles` in a separate diff... though I think it's fine for now

Leaving a quick comment based off of my initial read through the diff history, if I have any other comments after taking a more in-depth read I'll update here!

native/markdown/styles.js
63	Just out of curiosity: would it be better to leave this as `borderLeftColor: 'blockQuoteBorder'` and instead change the value of `blockQuoteBorder` in `native/themes/colors.js` for the dark mode? Just to make maintaining it easier.

removed memoization code to put in a seperate diff

Harbormaster completed remote builds in B12500: Diff 17155.Sep 28 2022, 12:14 PM

Seems like a good approach

Removing @abosh because it's his first day of class and he's not super available, and his feedback was addressed

This revision is now accepted and ready to land.Sep 29 2022, 12:12 PM

Closed by commit rCOMMed5ec298ee30: [native] Implement darker color tint to block quote container (authored by ginsu). · Explain WhySep 29 2022, 2:24 PM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMMed5ec298ee30: [native] Implement darker color tint to block quote container.

[native] Implement darker color tint to block quote container
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 17152

native/chat/inner-text-message.react.js

native/markdown/rules.react.js

native/markdown/styles.js

[native] Implement darker color tint to block quote containerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 17152

native/chat/inner-text-message.react.js

native/markdown/rules.react.js

native/markdown/styles.js

[native] Implement darker color tint to block quote container
ClosedPublic
Actions

Revision Contents
Changeset List