Page MenuHomePhabricator
Differential D5373

[native] Add spoiler style to native markdown styles
ClosedPublic
Actions

Authored by rohan on Oct 14 2022, 7:52 PM.
Tags
None
Referenced Files
F3376238: D5373.diff
Tue, Nov 26, 10:54 PM
Unknown Object (File)
Mon, Nov 25, 10:37 PM
Unknown Object (File)
Mon, Nov 25, 10:37 PM
Unknown Object (File)
Mon, Nov 25, 8:19 PM
Unknown Object (File)
Sun, Nov 24, 12:43 PM
Unknown Object (File)
Sun, Nov 24, 12:43 PM
Unknown Object (File)
Sun, Nov 24, 12:42 PM
Unknown Object (File)
Sun, Nov 24, 12:42 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
ginsu
• abosh
Commits
rCOMM4a38e57103a1: [native] Add spoiler style to native markdown styles
Summary

We add the styles for spoiler text on native by adding a style for both the text color and the background to make the spoilers uniquely
display.

https://linear.app/comm/issue/ENG-1984/add-spoiler-style-to-native-markdown-styles

Depends on D5347

Test Plan

Visually works as expected and the spoiler styles apply to only the spoiler text.

Spoiler Style.png (2×1 px, 246 KB)

Diff Detail

Repository
rCOMM Comm
Branch
spoiler
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan created this revision.Oct 14 2022, 7:52 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptOct 14 2022, 7:52 PM
rohan edited the summary of this revision. (Show Details)Oct 14 2022, 7:53 PM
rohan edited the test plan for this revision. (Show Details)
rohan attached a referenced file: F200186: Spoiler Style.png. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 14 2022, 7:55 PM
Harbormaster failed remote builds in B12813: Diff 17597!
Harbormaster completed remote builds in B12813: Diff 17597.Oct 14 2022, 8:03 PM
rohan requested review of this revision.Oct 14 2022, 8:03 PM
rohan edited the summary of this revision. (Show Details)Oct 14 2022, 8:03 PM
rohan added a parent revision: D5347: Add spoiler rule to native in fullMarkdownRules.
rohan edited the summary of this revision. (Show Details)
ginsu requested changes to this revision.Oct 15 2022, 10:51 AM

Code looks good, but I think about changing the black to something like a dark gray. I think the black looks a bit harsh with the black background outside the message container. Defer to @atul for second take as well

This revision now requires changes to proceed.Oct 15 2022, 10:51 AM
rohan added a comment.Oct 15 2022, 4:47 PM
In D5373#158958, @ginsu wrote:

Code looks good, but I think about changing the black to something like a dark gray. I think the black looks a bit harsh with the black background outside the message container. Defer to @atul for second take as well

Good point I agree after taking another look at it

rohan updated this revision to Diff 17601.EditedOct 15 2022, 4:48 PM

Update spoiler text color

Simulator Screen Shot - iPhone 14 Pro - 2022-10-15 at 19.49.06.png (2×1 px, 245 KB)

rohan attached a referenced file: F201172: Simulator Screen Shot - iPhone 14 Pro - 2022-10-15 at 19.49.06.png. (Show Details)Oct 15 2022, 4:49 PM
Harbormaster completed remote builds in B12817: Diff 17601.Oct 15 2022, 4:59 PM
atul accepted this revision.Oct 15 2022, 7:13 PM

Not sure if this is the "final" styling for spoilers, but looks good for now.

I think we might want to round the corners and kind of match the styling of code blocks (eg same color)

ashoat added a comment.Oct 17 2022, 5:31 AM

I think this is a fine style for now, we can revisit later

native/themes/colors.js
78–79 ↗(On Diff #17601)

Do we really need two separate styles for this? The whole point is to make the text not appear against the background, right?

ginsu accepted this revision.Oct 17 2022, 6:45 AM

color looks a lot better, I agree with @ashoat and I think we can get rid of spoilerText style. I would address that before landing

This revision is now accepted and ready to land.Oct 17 2022, 6:45 AM
rohan added inline comments.Oct 17 2022, 8:33 AM
native/themes/colors.js
78–79 ↗(On Diff #17601)

So I was originally thinking yes, because we wanted the text-color and background-color to be the same. Though now that I'm thinking about it, we can just have one common color spoiler and make both the text-color and background-color properties reference that color. Thanks for pointing it out - I can update this diff to do that

rohan updated this revision to Diff 17619.Oct 17 2022, 8:36 AM

Create just one style for the spoiler text and background

Harbormaster completed remote builds in B12834: Diff 17619.Oct 17 2022, 8:45 AM
rohan added a child revision: D5383: [native] Display the spoiler content on tap.Oct 17 2022, 12:31 PM
rohan updated this revision to Diff 18278.Nov 9 2022, 10:05 AM

Rebase and resolve conflicts

Harbormaster completed remote builds in B13327: Diff 18278.Nov 9 2022, 10:19 AM
rohan mentioned this in D5599: [web] Add spoiler style to MarkdownSpoiler.Nov 10 2022, 9:56 AM
rohan mentioned this in rCOMM343181cad8b6: [web] Add spoiler style to MarkdownSpoiler.Nov 16 2022, 9:06 AM
Closed by commit rCOMM4a38e57103a1: [native] Add spoiler style to native markdown styles (authored by rohan). · Explain WhyNov 30 2022, 2:47 PM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM4a38e57103a1: [native] Add spoiler style to native markdown styles.

Revision Contents
Changeset List

PathSize
native/
markdown/
6 lines
4 lines
themes/
2 lines

Diff 17619

View Options

native/markdown/rules.react.js

Loading...
View Options

native/markdown/styles.js

Loading...
View Options

native/themes/colors.js

Loading...