Page MenuHomePhabricator
Differential D5865

[native] Break spoiler text into individual components for rendering
AbandonedPublic
Actions

Authored by rohan on Dec 13 2022, 6:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 8, 4:43 PM
Unknown Object (File)
Mon, Apr 8, 5:55 AM
Unknown Object (File)
Mon, Apr 8, 5:55 AM
Unknown Object (File)
Mon, Apr 8, 5:55 AM
Unknown Object (File)
Sat, Apr 6, 10:48 AM
Unknown Object (File)
Wed, Apr 3, 2:16 AM
Unknown Object (File)
Mar 5 2024, 7:18 PM
Unknown Object (File)
Mar 5 2024, 7:18 PM
View All 41 Files
Subscribers
tomek

Details

Reviewers
ashoat
atul
michal
ginsu
Summary

Currently, if we wrap spoilers in a GestureTouchableOpacity, long spoilers will force a line break both before and after, causing some really bad UI. The purpose of this diff is to handle one of the potential solutions to this problem, where we tokenize the spoiler text by splitting each word into it's own Text component encompassed by a GestureTouchableOpacity. This becomes a little complicated with nested markdown within spoilers, so this is explained by the in-line comments. This isn't in a state to be landed, but it helps demonstrate the exploration process.

All context is found in the Linear task: https://linear.app/comm/issue/ENG-2446/[exploration]-break-spoiler-text-into-individual-text-elements

Test Plan

All of the testing videos / screenshots for both iOS and Android can be found in the Linear issue.

Diff Detail

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

Event Timeline

rohan created this revision.Dec 13 2022, 6:37 PM
Herald added a subscriber: tomek. · View Herald TranscriptDec 13 2022, 6:37 PM
rohan edited the summary of this revision. (Show Details)Dec 13 2022, 6:37 PM
rohan edited the summary of this revision. (Show Details)
rohan edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B14139: Diff 19339.Dec 13 2022, 6:50 PM
rohan requested review of this revision.Dec 13 2022, 6:50 PM
ashoat requested changes to this revision.Dec 13 2022, 6:53 PM

Thanks for putting this up as an explainer! Removing from reviewers' queues since it's not in a state to be reviewed yet

This revision now requires changes to proceed.Dec 13 2022, 6:53 PM
ashoat added a comment.Dec 13 2022, 7:04 PM

Main things to iterate on here:

  1. Markdown within spoilers can't be wrapped normally right now. We probably need to break those down into individual Texts as well
  2. Need to find a way to preserve styles
  3. Need to decide where we might want to use this. Makes sense for MarkdownLink on iOS/Android, but not sure about MarkdownSpoiler since it causes the individual spoiler words to be "leaked" on press/hold
  4. Need to avoid deleting the old method, assuming we want to continue using it for MarkdownSpoiler
native/markdown/markdown-spoiler.react.js
101

Can we split by ' ' but keep it in the resultant tokens? We then wouldn't have to append it again later

rohan added a comment.Dec 14 2022, 12:41 PM
In D5865#175478, @ashoat wrote:

Main things to iterate on here:

  1. Markdown within spoilers can't be wrapped normally right now. We probably need to break those down into individual Texts as well

Agreed - seems like long markdown within the spoiler does not wrap properly. Let me take a look in breaking them down the same way, but maintaining the markdown styles.

  1. Need to find a way to preserve styles

I think this will be out of scope for this, but if we resolve the line breaking problem with GestureTouchableOpacity, we should be able to attempt your solution of the Stylesheet.compose with some kind of context.

  1. Need to decide where we might want to use this. Makes sense for MarkdownLink on iOS/Android, but not sure about MarkdownSpoiler since it causes the individual spoiler words to be "leaked" on press/hold

One solution to prevent the spoilers from being leaked on press/hold is that we need to target the styled spoiler text. We can make the text transparent while it is hidden, instead of black, and this way (see the Linear task for a screenshot) the text is not visible until that style is no longer applied once the spoiler is revealed.

  1. Need to avoid deleting the old method, assuming we want to continue using it for MarkdownSpoiler

Got it, this definitely isn't in a state to land so I'll work on keeping track of what needs to be kept / removed if this is a path we want to go down.

native/markdown/markdown-spoiler.react.js
101

Let me look a little into this! It might be possible, it depends how the words and spaces will be rendered when mapped into Text components and I don't know the answer off the top of my head.

rohan added inline comments.Dec 16 2022, 12:06 PM
native/markdown/markdown-spoiler.react.js
101

We could probably do child.split(/(\s+)/) and store that into words to preserve all of the spacing.

Context: https://stackoverflow.com/questions/26425637/javascript-split-string-with-white-space

rohan abandoned this revision.Mar 1 2023, 7:59 AM

Abandoning this since this isn't actively being worked on, but it can be revisited and reopened at a later point if needed

Revision Contents
Changeset List

PathSize
native/
chat/
25 lines
markdown/
71 lines
5 lines

Diff 19339

View Options

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

Loading...
View Options

native/markdown/markdown-spoiler.react.js

Loading...
View Options

native/markdown/rules.react.js

Loading...