Page MenuHomePhabricator
Differential D5601

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

Authored by rohan on Nov 10 2022, 10:32 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 18, 1:37 PM
Unknown Object (File)
Fri, Apr 18, 1:09 PM
Unknown Object (File)
Fri, Apr 18, 10:38 AM
Unknown Object (File)
Fri, Apr 18, 10:30 AM
Unknown Object (File)
Fri, Apr 18, 8:05 AM
Unknown Object (File)
Fri, Apr 18, 1:16 AM
Unknown Object (File)
Fri, Apr 18, 12:35 AM
Unknown Object (File)
Thu, Apr 17, 7:53 PM
View All Files
Subscribers
• abosh
ashoat
tomek

Details

Reviewers
atul
ginsu
ashoat
Commits
rCOMM249762b2bda0: [web] Reveal the spoiler content on click within a message
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 created this revision.Nov 10 2022, 10:32 AM
Herald added subscribers: • abosh, tomek, ashoat. · View Herald TranscriptNov 10 2022, 10:32 AM
rohan edited the summary of this revision. (Show Details)Nov 10 2022, 10:33 AM
rohan edited the test plan for this revision. (Show Details)
rohan added a parent revision: D5599: [web] Add spoiler style to MarkdownSpoiler.
rohan attached a referenced file: F231875: Reveal Spoilers Web.mov. (Show Details)
Harbormaster completed remote builds in B13372: Diff 18334.Nov 10 2022, 10:46 AM
rohan requested review of this revision.Nov 10 2022, 10:46 AM
rohan updated this revision to Diff 18364.Nov 10 2022, 5:35 PM

Rebase on MarkdownSpoiler changes

Harbormaster completed remote builds in B13389: Diff 18364.Nov 10 2022, 5:48 PM
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
rohan updated this revision to Diff 18371.Nov 10 2022, 9:11 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

Harbormaster completed remote builds in B13396: Diff 18371.Nov 10 2022, 9:25 PM
rohan added a child revision: D5617: [web] Prevent hidden spoiler content from being revealed by text/cursor highlighting.Nov 10 2022, 10:06 PM
ginsu accepted this revision.Nov 11 2022, 9:15 AM
ashoat accepted this revision.Nov 11 2022, 1:04 PM
rohan updated this revision to Diff 18395.Nov 11 2022, 5:08 PM

Rebase on previous diff changes

Harbormaster completed remote builds in B13411: Diff 18395.Nov 11 2022, 5:21 PM
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
rohan added a comment.Nov 12 2022, 8:41 AM
In D5601#166595, @ashoat wrote:

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

rohan updated this revision to Diff 18397.Nov 12 2022, 8:42 AM

Revert to prior accepted revision

Harbormaster completed remote builds in B13413: Diff 18397.Nov 12 2022, 8:56 AM
ashoat accepted this revision.Nov 12 2022, 9:54 AM
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

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
atul added inline comments.Nov 14 2022, 5:12 PM
web/markdown/markdown.css
85–97

*as I'm suggesting.

ashoat added inline comments.Nov 14 2022, 5:35 PM
web/markdown/markdown-spoiler.react.js
16

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)

rohan added inline comments.Nov 15 2022, 6:29 AM
web/markdown/markdown-spoiler.react.js
16

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

web/markdown/markdown.css
85–97

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!

rohan updated this revision to Diff 18433.EditedNov 15 2022, 8:08 AM

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 ^

Harbormaster completed remote builds in B13435: Diff 18433.Nov 15 2022, 8:23 AM
atul added inline comments.Nov 15 2022, 3:41 PM
web/markdown/markdown.css
85–97

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

atul accepted this revision.Nov 15 2022, 3:51 PM
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
Closed by commit rCOMM249762b2bda0: [web] Reveal the spoiler content on click within a message (authored by rohan). · Explain WhyNov 16 2022, 9:10 AM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM249762b2bda0: [web] Reveal the spoiler content on click within a message.

Revision Contents
Changeset List

PathSize
web/
markdown/
12 lines
13 lines

Diff 18397

View Options

web/markdown/markdown-spoiler.react.js

Show First 20 Lines • Show All 48 Lines • ▼ Show 20 Lines): React.Node {
const onPressSidebar = useAnimatedNavigateToSidebar(route.params.item); const onPressSidebar = useAnimatedNavigateToSidebar(route.params.item);
const renderSidebarIcon = React.useCallback( const renderSidebarIcon = React.useCallback(
style => ( style => (
<SWMansionIcon name="message-circle-lines" style={style} size={16} /> <SWMansionIcon name="message-circle-lines" style={style} size={16} />
), ),
[], [],
); );
const { item } = route.params;
const { messageInfo } = item;
inkaUnsubmitted
Done
Inline Actions

can't this be const { messageInfo } = route.params.item;? I don't think you use item anywhere afterwards

inka: can't this be `const { messageInfo } = route.params.item;`? I don't think you use `item`…
const onPressEdit = () => {
invariant(
inputState,
'inputState should be set in TextMessageTooltipModal.onPressEdit',
);
inputState.setEditedMessageID(messageInfo.id);
};
inkaUnsubmitted
Done
Inline Actions

Shouldn't this be inside React.useCallback?

inka: Shouldn't this be inside React.useCallback?
const renderEditIcon = React.useCallback(
style => <SWMansionIcon name="edit-1" style={style} size={16} />,
[],
);
const onPressCopy = React.useCallback(() => { const onPressCopy = React.useCallback(() => {
Clipboard.setString(text); Clipboard.setString(text);
setTimeout(confirmCopy); setTimeout(confirmCopy);
}, [text]); }, [text]);
const renderCopyIcon = React.useCallback( const renderCopyIcon = React.useCallback(
style => <SWMansionIcon name="copy" style={style} size={16} />, style => <SWMansionIcon name="copy" style={style} size={16} />,
[], [],
); );
Show All 16 Lines <>
<TooltipItem <TooltipItem
id="sidebar" id="sidebar"
text="Thread" text="Thread"
onPress={onPressSidebar} onPress={onPressSidebar}
renderIcon={renderSidebarIcon} renderIcon={renderSidebarIcon}
key="sidebar" key="sidebar"
/> />
<TooltipItem <TooltipItem
id="edit"
text="Edit"
onPress={onPressEdit}
renderIcon={renderEditIcon}
key="edit"
/>
<TooltipItem
id="copy" id="copy"
text="Copy" text="Copy"
onPress={onPressCopy} onPress={onPressCopy}
renderIcon={renderCopyIcon} renderIcon={renderCopyIcon}
key="copy" key="copy"
/> />
<TooltipItem <TooltipItem
id="report" id="report"
Show All 17 Lines
View Options

web/markdown/markdown.css

Show First 20 Lines • Show All 48 Lines • ▼ Show 20 Lines): React.Node {
const onPressSidebar = useAnimatedNavigateToSidebar(route.params.item); const onPressSidebar = useAnimatedNavigateToSidebar(route.params.item);
const renderSidebarIcon = React.useCallback( const renderSidebarIcon = React.useCallback(
style => ( style => (
<SWMansionIcon name="message-circle-lines" style={style} size={16} /> <SWMansionIcon name="message-circle-lines" style={style} size={16} />
), ),
[], [],
); );
const { item } = route.params;
const { messageInfo } = item;
inkaUnsubmitted
Done
Inline Actions

can't this be const { messageInfo } = route.params.item;? I don't think you use item anywhere afterwards

inka: can't this be `const { messageInfo } = route.params.item;`? I don't think you use `item`…
const onPressEdit = () => {
invariant(
inputState,
'inputState should be set in TextMessageTooltipModal.onPressEdit',
);
inputState.setEditedMessageID(messageInfo.id);
};
inkaUnsubmitted
Done
Inline Actions

Shouldn't this be inside React.useCallback?

inka: Shouldn't this be inside React.useCallback?
const renderEditIcon = React.useCallback(
style => <SWMansionIcon name="edit-1" style={style} size={16} />,
[],
);
const onPressCopy = React.useCallback(() => { const onPressCopy = React.useCallback(() => {
Clipboard.setString(text); Clipboard.setString(text);
setTimeout(confirmCopy); setTimeout(confirmCopy);
}, [text]); }, [text]);
const renderCopyIcon = React.useCallback( const renderCopyIcon = React.useCallback(
style => <SWMansionIcon name="copy" style={style} size={16} />, style => <SWMansionIcon name="copy" style={style} size={16} />,
[], [],
); );
Show All 16 Lines <>
<TooltipItem <TooltipItem
id="sidebar" id="sidebar"
text="Thread" text="Thread"
onPress={onPressSidebar} onPress={onPressSidebar}
renderIcon={renderSidebarIcon} renderIcon={renderSidebarIcon}
key="sidebar" key="sidebar"
/> />
<TooltipItem <TooltipItem
id="edit"
text="Edit"
onPress={onPressEdit}
renderIcon={renderEditIcon}
key="edit"
/>
<TooltipItem
id="copy" id="copy"
text="Copy" text="Copy"
onPress={onPressCopy} onPress={onPressCopy}
renderIcon={renderCopyIcon} renderIcon={renderCopyIcon}
key="copy" key="copy"
/> />
<TooltipItem <TooltipItem
id="report" id="report"
Show All 17 Lines