Page MenuHomePhabricator

[native] Maintain the revealed state of spoilers
ClosedPublic

Authored by rohan on Nov 4 2022, 12:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 6:57 AM
Unknown Object (File)
Wed, Dec 11, 5:44 PM
Unknown Object (File)
Tue, Dec 10, 5:55 PM
Unknown Object (File)
Thu, Dec 5, 8:27 PM
Unknown Object (File)
Mon, Dec 2, 5:14 PM
Unknown Object (File)
Mon, Dec 2, 4:31 PM
Unknown Object (File)
Mon, Dec 2, 3:00 PM
Unknown Object (File)
Mon, Dec 2, 3:00 PM

Details

Summary

This diff handles storing the spoiler reveal state so when a user taps on a message once the spoiler is revealed, it won't "re-hide" (the behavior that is present further down in this stack). With this diff, we can now keep track of what spoilers are revealed and what spoilers are still hidden, and the style of the Text component changes once a user reveals the spoiler.

ENG-2072: https://linear.app/comm/issue/ENG-2072/maintain-the-spoiler-state-upon-opening-textmessagetooltipmodal

Depends on D5515

Test Plan

Create a chat with two separate text messages, each with their own spoilers. The main test is to check whether the spoiler "reveal" state is maintained when the tooltip appears.

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

Rebase & Switch to using state.key as the unique identifer for each
MarkdownSpoiler component to keep track of what spoilers have been
revealed (and allowing duplicate content within spoilers to remain
separate)

Remove console.log statement

atul requested changes to this revision.Nov 9 2022, 1:11 PM

Left some questions/comments inline. Can we schedule a quick call sometime today/tomorrow/Friday so I can clarify a few things about the design here?

native/chat/text-message.react.js
254 ↗(On Diff #18285)

Do we really need to introduce spoilerIsBlockingTooltip here, or can we just pass spoilerPressActive directly into spoilerIsBlockingTooltip prop of TextMessage component?

native/markdown/markdown-context-provider.react.js
19–23 ↗(On Diff #18285)

If I understand correctly the outer [key: string] is for messageID and the inner [key: number] is for the spoiler identifier, is that right?

native/markdown/markdown-spoiler.react.js
23–25 ↗(On Diff #18285)

Can we use useMemo instead so we don't have to imperatively update styleBasedOnState with setStyleBasedOnState?

35–38 ↗(On Diff #18285)

Hm this doesn't seem quite right that we're early returning based on the value of a style?

This revision now requires changes to proceed.Nov 9 2022, 1:11 PM
native/chat/text-message.react.js
254 ↗(On Diff #18285)

Good catch, makes sense just passing spoilerPressActive

native/markdown/markdown-context-provider.react.js
19–23 ↗(On Diff #18285)

Yeah, exactly! So we get {messageID: {identifier: boolean} }

native/markdown/markdown-spoiler.react.js
23–25 ↗(On Diff #18285)

This is done in D5541 in the most latest revision

35–38 ↗(On Diff #18285)

It's to check if the spoiler is revealed, I can't check spoilerRevealed[messageID][identifier] === true since that gets set below.

Pass spoilerPressActive prop

atul requested changes to this revision.Nov 14 2022, 1:58 PM

Let's get rid of spoilerPressActive/setSpoilerPressActive and use gesture handlers to block Tooltip from appearing when Spoiler is pressed instead.

The use of styleBasedOnState also needs to be fixed.


In order to get this diff landed:

  • Remove spoilerPressActive and setSpoilerPressActive altogether for now.
  • Stop using useState for styleBasedOnState and instead compute it in a useMemo or something (we can walk through doing this on call tomorrow)
native/markdown/markdown-spoiler.react.js
23–25 ↗(On Diff #18295)

We don't want to be storing the style in a useState like this... we'll want to compute the style based on state.

35–38 ↗(On Diff #18295)

We don't want to be using styles like this in our logic... styles should just be a function of the state.

We should explicitly introduce state that captures what we're trying to use styleBasedOnState for. But we shouldn't use styleBasedOnState since styling is a "side-effect" of state... not the actual state

47 ↗(On Diff #18295)

Yeah we don't want to be imperatively setting styles like this. We should set the dependencies and let it be computed declaratively

This revision now requires changes to proceed.Nov 14 2022, 1:58 PM
atul requested changes to this revision.Nov 15 2022, 2:35 PM

Not sure we need isRevealed state

native/markdown/markdown-spoiler.react.js
29–31 ↗(On Diff #18437)

Hm, there's got to be a better way to handle this. I'm afraid that if parsedSpoilerIdentifier is ever actually set to -1 it would lead to strange behavior. Let's leave this as is... but can we create a Linear task or something to see if we can handle this case more gracefully?

33–35 ↗(On Diff #18437)

Do we need to keep track of this with useState or can we just use spoilerRevealed?.[messageID]?.[parsedSpoilerIdentifier] ?? false and setSpoilerRevealed directly?


We can still pull spoilerRevealed?.[messageID]?.[parsedSpoilerIdentifier] ?? false out into a variable called isRevealed or something

eg

const isRevealed = spoilerRevealed?.[messageID]?.[parsedSpoilerIdentifier] ?? false;

but I don't think we need to use useState and don't really have much use for setIsRevealed

This revision now requires changes to proceed.Nov 15 2022, 2:35 PM
native/markdown/markdown-spoiler.react.js
29–31 ↗(On Diff #18437)
33–35 ↗(On Diff #18437)

I can definitely use React.useMemo() since we wouldn't need the setIsRevealed too much like you mentioned. My reasoning for React.useMemo() is we want the isRevealed state to recompute once the dependency (spoilerRevealed) is updated with the new spoiler.

Use React.useMemo() instead of React.useState()

atul requested changes to this revision.Nov 15 2022, 6:59 PM

We can pull isRevealed out of hooks altogether

native/markdown/markdown-spoiler.react.js
33–35 ↗(On Diff #18437)

With

const isRevealed = spoilerRevealed?.[messageID]?.[parsedSpoilerIdentifier] ?? false;

isRevealed will recompute when spoilerRevealed is updated.

Any change in markdownContext?.spoilerRevealed should trigger everything below in the component tree to get recomputed: https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/#context-and-rendering-behavior

Is this not inline with the behavior you're observing?

This revision now requires changes to proceed.Nov 15 2022, 6:59 PM
In D5539#167228, @atul wrote:

Is this not inline with the behavior you're observing?

Ah, looks like my simulator was just out of sync so it wasn't updating. Thanks for the link I read through it and makes much more sense.

Remove unneeded React.useMemo()

This revision is now accepted and ready to land.Nov 16 2022, 5:32 AM

Prevent a if(0) condition from occuring

Rebase and extract messageKey now

ashoat requested changes to this revision.Nov 22 2022, 6:14 AM

Feel free to re-request review if there is some legitimate scenario in which markdownContext will not be defined

native/markdown/markdown-spoiler.react.js
27 ↗(On Diff #18700)

In what scenario will markdownContext not be defined? If there is no such scenario, please add an invariant and remove all conditions (eg. the optional chaining here, the first two conditions in the if statement on lines 48-53, etc.)

This revision now requires changes to proceed.Nov 22 2022, 6:14 AM
native/markdown/markdown-spoiler.react.js
27 ↗(On Diff #18700)

Looking at this further, I think the only reason you need to contemplate a scenario where markdownContext is not defined is because in D5515, you are rendering MarkdownContextProvider as a parent of the main TextMessage, but not as a parent of the InnerTextMessage modal. I assume that was not intentional... more in D5515

native/markdown/markdown-spoiler.react.js
27 ↗(On Diff #18700)

Oops... actually, it is a parent of both the main TextMessage as well as the TextMessageTooltipModal, but it is NOT a parent of ChatContextProvider, which renders ChatItemtHeightMeasurer which renders InnerTextMessage. Anyways, more context in D5515

native/markdown/markdown-spoiler.react.js
27 ↗(On Diff #18700)

Responded to your comments in D5515, but this makes sense - I appreciate the explanation. I put up a revision to relocate MarkdownContextProvider, so I'll go ahead and resolve the need for the optional chaining and if conditions for this diff now

Add an invariant to check for MarkdownContext, and remove the optional chaining & conditional checks for MarkdownContext.

This revision is now accepted and ready to land.Nov 22 2022, 5:12 PM

Remove conditional checks for messageKey from MessageContext

I don't think this diff does what its description says it does anymore, specifically:

This is necessary in order to do two things: 1) If the spoiler has not been revealed, and a user taps on it, we should prevent the tooltip from appearing and just display the message contents, and 2) If the spoiler is already revealed, we want to allow the tooltip to appear on tap.

@rohan, could you revise the diff description?

native/markdown/markdown-spoiler.react.js
71 ↗(On Diff #18785)

Let's revert this back to a Text... GestureTouchableOpacity isn't going to work given @michal's prior observations around line breaks

rohan retitled this revision from [native] Prevent tooltip from appearing on spoiler tap if spoiler is not yet revealed to [native] Maintain the spoiler reveal state on tap.Nov 26 2022, 8:27 AM
rohan edited the summary of this revision. (Show Details)
rohan edited the test plan for this revision. (Show Details)
rohan retitled this revision from [native] Maintain the spoiler reveal state on tap to [native] Maintain the revealed state of spoilers.

Revert back to using Text instead of GestureTouchableOpacity

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