Details

Reviewers

atul
ginsu
• abosh
ashoat
tomek

Commits

rCOMM6a03fa1f9797: Add spoiler rule to native in fullMarkdownRules

Summary

Add the spoiler rule for native to use the capture and parse functions from the previous diff, and return a <Text> ... </Text> React element with a spoiler style.

Linear Issue: https://linear.app/comm/issue/ENG-1983/add-spoiler-rule-to-fullmarkdownrules-in-nativemarkdownrulesreactjs

Depends on D5346

Test Plan

Tested the spoiler text being captured with a temporary spoiler style, and I will attach the photos below. ENG-1984 will handle the styling for the spoiler text.

Example:

Simulator Screen Shot - iPhone 14 Pro - 2022-10-11 at 14.39.22.png (2×1 px, 232 KB)

Simulator Screen Shot - iPhone 14 Pro - 2022-10-11 at 14.39.33.png (2×1 px, 276 KB)

Diff Detail

Repository

rCOMM Comm

Branch

spoiler

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Oct 11 2022, 11:43 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptOct 11 2022, 11:43 AM

rohan edited the summary of this revision. (Show Details)Oct 11 2022, 11:44 AM

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

rohan added a parent revision: D5346: Add the match and parse functions for spoiler text.

rohan attached a referenced file: F196188: Simulator Screen Shot - iPhone 14 Pro - 2022-10-11 at 14.39.33.png. (Show Details)

rohan attached a referenced file: F196187: Simulator Screen Shot - iPhone 14 Pro - 2022-10-11 at 14.39.22.png. (Show Details)

Harbormaster completed remote builds in B12736: Diff 17497.Oct 11 2022, 11:51 AM

rohan requested review of this revision.Oct 11 2022, 11:51 AM

ginsu requested changes to this revision.Oct 11 2022, 12:41 PM

ginsu added inline comments.

native/markdown/styles.js
14–18 ↗	(On Diff #17497)	I would get rid of this until this is more complete. From what I have been told, a diff should be able to be landed at any time, so we probably don't want any placeholder code

This revision now requires changes to proceed.Oct 11 2022, 12:41 PM

rohan added inline comments.Oct 12 2022, 4:57 AM

native/markdown/styles.js
14–18 ↗	(On Diff #17497)	Good point - I'm going to be doing some testing for the previous diff to address some comments this afternoon, so once that's done I'll update this diff to remove the placeholder color.

Rebase

Harbormaster completed remote builds in B12755: Diff 17523.Oct 12 2022, 12:16 PM

Use the parse and match methods already present in SimpleMarkdown.

@ashoat This is what we discussed in our sync today

Harbormaster completed remote builds in B12756: Diff 17524.Oct 12 2022, 12:39 PM

Rebase on spoiler branch

Harbormaster completed remote builds in B12763: Diff 17532.Oct 12 2022, 2:12 PM

Adding @ashoat as reviewer since

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
(https://phab.comm.dev/D4999)

And I'd still not consider myself fully up to speed with the markdown area of the codebase.