Page MenuHomePhabricator

[web] Reveal the spoiler content on click within a message
ClosedPublic

Authored by rohan on Nov 10 2022, 10:32 AM.
Tags
None
Referenced Files
F3378675: D5601.diff
Wed, Nov 27, 12:23 PM
F3375932: D5601.id18395.diff
Tue, Nov 26, 9:28 PM
Unknown Object (File)
Sat, Nov 23, 9:56 PM
Unknown Object (File)
Sat, Nov 23, 9:52 PM
Unknown Object (File)
Sat, Nov 23, 9:33 PM
Unknown Object (File)
Sat, Nov 23, 9:03 PM
Unknown Object (File)
Sat, Nov 23, 9:02 PM
Unknown Object (File)
Sat, Nov 23, 8:25 PM

Details

Summary

We add an onClick function to allow the spoiler content to be revealed on click. This onClick function then removes the spoiler class and animates the "reveal" effect to display the text content. The spoiler tags will not be re-rendered until the messages are unmounted.

https://linear.app/comm/issue/ENG-2192/reveal-the-spoiler-content-on-click-within-a-message

Depends on D5599

Test Plan

Spoiler tags are revealed as expected, with a timed animation to fade the background and they get re-rendered after leaving and re-opening the chat.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

Rebase on MarkdownSpoiler changes

ashoat requested changes to this revision.Nov 10 2022, 6:49 PM
ashoat added inline comments.
web/markdown/markdown-spoiler.react.js
16–19 ↗(On Diff #18364)

You should almost never define a callback in a React component without using React.useCallback

16–19 ↗(On Diff #18364)

You should absolutely never mutate CSS classes like this in React. This is a huge anti-pattern

This revision now requires changes to proceed.Nov 10 2022, 6:49 PM

Use React.useCallback and React.useState

web/markdown/markdown-spoiler.react.js
16–19 ↗(On Diff #18364)

Ah right, let me put up a revision to fix both of these

Rebase on previous diff changes

ashoat requested changes to this revision.Nov 12 2022, 6:44 AM

This is the old version. You're likely doing something wrong in Git... encourage you to think carefully about how you're using Git / arc

This revision now requires changes to proceed.Nov 12 2022, 6:44 AM

This is the old version. You're likely doing something wrong in Git... encourage you to think carefully about how you're using Git / arc

Ah, sorry about that - let me fix that

Revert to prior accepted revision

atul requested changes to this revision.Nov 14 2022, 5:12 PM

Can we try using transition instead of animation to simplify things/keep things consistent with our codebase?

(Also like I mentioned in D5599, we should pull the hardcoded hex values out to CSS variables in web/theme.css and consume them here.)

web/markdown/markdown.css
85–97 ↗(On Diff #18397)

Can we use transition here instead of animation?

My understanding is that animation and @keyframes are helpful for more complex animations, but transition is simpler and should work fine for this.

Feel free to push back if I'm missing something or transition doesn't work as I'm suggested.

This revision now requires changes to proceed.Nov 14 2022, 5:12 PM
web/markdown/markdown.css
85–97 ↗(On Diff #18397)

*as I'm suggesting.

web/markdown/markdown-spoiler.react.js
16 ↗(On Diff #18397)

Arguably same point as here can be made here... instead of the state being the className, it would be better for readaibility to have the state be something human-readable that describes what state the component is in (eg. isRevealed: boolean)

web/markdown/markdown-spoiler.react.js
16 ↗(On Diff #18397)

Agreed, I'll make that change in the revision!

web/markdown/markdown.css
85–97 ↗(On Diff #18397)

From my understanding of transition, there isn't a simple / clean way to use transition when we're handling a click. Transition would've seemed better if spoilers were revealed on hover or active, but since we want a persistent style change, my understanding was a keyframe would be the better solution for this.

For example, if it was hover we could do:

span.spoiler {
  background: #33332c;
  color: #33332c;
  cursor: pointer;
  transition: color 1s, background-color 1s;
}

span.spoiler:hover {
   color: white;
   background-color: transparent;
}

With a click on the spoiler though, we're changing the spoiler style so we just want to simulate the spoiler tag fading away until it defaults to the new style, ie just transparent and white text

Happy to work more on transition though if you feel like it's the better move here, as I'm not super aware of transition besides that use case!

Use CSS Variables and have a isRevealed state to describe the state of the spoiler.

@atul just put up a revision to not block revisions on the diff stack, but still open to transition based on my inline comments above ^

web/markdown/markdown.css
85–97 ↗(On Diff #18397)

Understood! Appreciate the in-depth explanation... should be good as-is

atul added inline comments.
web/markdown/markdown-spoiler.react.js
18–23 ↗(On Diff #18433)

Could maybe make this a bit more concise with a ternary... but up to you

const styleBasedOnSpoilerState = React.useMemo(
  () => (isRevealed ? css.revealSpoilerAnimation : css.spoiler),
  [isRevealed],
);
This revision is now accepted and ready to land.Nov 15 2022, 3:51 PM