Details

Reviewers

atul
ginsu
• abosh

Commits

rCOMM171dcad4dbd1: [native] Display the spoiler content on tap

Summary

We render a new Spoiler component for text content parsed to be spoilers. Upon tap, this Spoiler component reveals the text beneath
(https://linear.app/comm/issue/ENG-2011/display-the-spoiler-content-on-tap-within-messagelist-[native]). This diff will not be landed until ENG-2072 is addressed, to resolve the spoiler state not being remembered upon message tap.

Depends on D5373

Test Plan

Spoiler messages are revealed when tapping the spoiler content.

Diff Detail

Repository

rCOMM Comm

Branch

spoiler

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Oct 17 2022, 12:29 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptOct 17 2022, 12:29 PM

rohan edited the summary of this revision. (Show Details)Oct 17 2022, 12:31 PM

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

rohan added a parent revision: D5373: [native] Add spoiler style to native markdown styles.

rohan attached a referenced file: F202315: demo.mp4. (Show Details)Oct 17 2022, 12:36 PM

Harbormaster completed remote builds in B12838: Diff 17625.Oct 17 2022, 12:40 PM

rohan requested review of this revision.Oct 17 2022, 12:40 PM

ginsu added inline comments.Oct 18 2022, 1:05 PM

native/components/spoiler.react.js
27 ↗	(On Diff #17625)	nit: For some reason, this feels weird to me, I would write it like this I'd see what @atul says as well

ginsu requested changes to this revision.Oct 18 2022, 1:05 PM

This revision now requires changes to proceed.Oct 18 2022, 1:05 PM

rohan added inline comments.Oct 18 2022, 2:05 PM

native/components/spoiler.react.js
27 ↗	(On Diff #17625)	Thanks for pointing it out, I think it looks better with your suggestion

Update styling logic

Harbormaster completed remote builds in B12858: Diff 17648.Oct 18 2022, 2:17 PM

Update ReactElement import

Harbormaster completed remote builds in B12860: Diff 17650.Oct 18 2022, 2:31 PM

Looks good, but we should hold off on landing until we figure out the stuff discussed here: https://linear.app/comm/issue/ENG-2072/maintain-the-spoiler-state-upon-opening-textmessagetooltipmodal

We probably don't want to make a release with broken spoiler support

native/components/spoiler.react.js
24 ↗	(On Diff #17650)	We could call this `memoizedSpoiler` or something

nice thanks for addressing my feedback!

This revision is now accepted and ready to land.Oct 19 2022, 11:07 AM

In D5383#159554, @atul wrote:

Looks good, but we should hold off on landing until we figure out the stuff discussed here: https://linear.app/comm/issue/ENG-2072/maintain-the-spoiler-state-upon-opening-textmessagetooltipmodal

We probably don't want to make a release with broken spoiler support

Yeah that's the plan, after ENG-2072 is approved then I'll land the diffs in this stack and that should allow for working spoilers on native. Thanks for the feedback, I'll address it in my next revision

Rename memoizedText to memoizedSpoiler

atul accepted this revision.Oct 19 2022, 11:31 AM

Harbormaster completed remote builds in B12872: Diff 17663.Oct 19 2022, 11:32 AM

I actually don't think it's crazy to launch spoilers with the issue that the spoilers get re-hidden if you select a message

In D5383#159602, @ashoat wrote:

I actually don't think it's crazy to launch spoilers with the issue that the spoilers get re-hidden if you select a message

I think either solution is fine - we can hold off on landing until the ENG-2072 (https://linear.app/comm/issue/ENG-2072/maintain-the-spoiler-state-upon-opening-textmessagetooltipmodal) is done, or land what is in this stack now! Open to either

rohan added a child revision: D5446: [native] Rename MarkdownLinkContext to MarkdownContext.Oct 20 2022, 12:06 PM

rohan mentioned this in D5476: [native] Refactor Spoiler component to MarkdownSpoiler.Oct 25 2022, 5:30 AM

Update Spoiler to MarkdownSpoiler

Harbormaster completed remote builds in B13027: Diff 17893.Oct 25 2022, 5:48 AM

Rebase and resolve conflicts

Harbormaster completed remote builds in B13328: Diff 18279.Nov 9 2022, 10:20 AM

Rebase (pending testing since my dev environment isn't working properly
again)

Harbormaster completed remote builds in B13892: Diff 19032.Nov 30 2022, 12:30 PM

Closed by commit rCOMM171dcad4dbd1: [native] Display the spoiler content on tap (authored by rohan). · Explain WhyNov 30 2022, 2:48 PM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM171dcad4dbd1: [native] Display the spoiler content on tap.