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

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

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

Rebase on previous diff changes

Harbormaster completed remote builds in B13411: Diff 18395.Nov 11 2022, 5:21 PM

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

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

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

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!

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.

Diff 18397

View Options

web/markdown/markdown-spoiler.react.js

	// @flow			// @flow

	import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';			import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
				import invariant from 'invariant';
	import * as React from 'react';			import * as React from 'react';
	import { Text } from 'react-native';			import { Alert, Text } from 'react-native';
	import { TouchableOpacity } from 'react-native-gesture-handler';			import { TouchableOpacity } from 'react-native-gesture-handler';

				import type { NavigationRoute } from '../navigation/route-names';
	import { useColors } from '../themes/colors.js';			import { useColors } from '../themes/colors.js';

	type Props = React.ElementConfig<typeof BaseHeaderBackButton>;			type ChangeRolesHeaderLeftButtonProps = {
	function ChangeRolesHeaderLeftButton(props: Props): React.Node {			+route: NavigationRoute<'ChangeRolesScreen'>,
				...React.ElementConfig<typeof BaseHeaderBackButton>,
				};

				function ChangeRolesHeaderLeftButton(
				props: ChangeRolesHeaderLeftButtonProps,
				): React.Node {
				const { memberInfo, role: selectedRole } = props.route.params;
				const { role: memberRole } = memberInfo;

				const onCancel = React.useCallback(() => {
				const { onPress } = props;
				invariant(onPress, 'onPress must be defined');

				if (selectedRole === memberRole) {
				onPress();
				return;
				}

				Alert.alert(
				'Unsaved Changes',
				'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
				[
				{ text: 'Cancel', onPress },
				{ text: 'Stay', style: 'cancel' },
				],
				{ cancelable: true },
				);
				}, [memberRole, props, selectedRole]);
				rohanAuthorUnsubmitted Done Inline Actions I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes. Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted) rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
				ashoatUnsubmitted Not Done Inline Actions What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
				rohanAuthorUnsubmitted Done Inline Actions Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues editing while `Discard edit` closes the alert and exits editing rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
				ashoatUnsubmitted Not Done Inline Actions I agree we shouldn't use the term `Cancel` here... how about `Leave`? I don't think we need `{ cancelable: true }`... we can just default to `Stay` if the user dismisses the `Alert` Can we try to match copy for title / description with the edit-message workflow? ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
				rohanAuthorUnsubmitted Done Inline Actions Yeah that makes sense, I'll update the diff rohan: Yeah that makes sense, I'll update the diff

	const { panelForegroundSecondaryLabel } = useColors();			const { panelForegroundSecondaryLabel } = useColors();
	const labelStyle = React.useMemo(			const labelStyle = React.useMemo(
	() => ({			() => ({
	color: panelForegroundSecondaryLabel,			color: panelForegroundSecondaryLabel,
	marginLeft: 10,			marginLeft: 10,
	}),			}),
	[panelForegroundSecondaryLabel],			[panelForegroundSecondaryLabel],
	);			);

	return (			return (
	<TouchableOpacity onPress={props.onPress}>			<TouchableOpacity onPress={onCancel}>
	<Text style={labelStyle}>Cancel</Text>			<Text style={labelStyle}>Cancel</Text>
	</TouchableOpacity>			</TouchableOpacity>
	);			);
	}			}

	export default ChangeRolesHeaderLeftButton;			export default ChangeRolesHeaderLeftButton;

View Options

web/markdown/markdown.css

	// @flow			// @flow

	import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';			import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
				import invariant from 'invariant';
	import * as React from 'react';			import * as React from 'react';
	import { Text } from 'react-native';			import { Alert, Text } from 'react-native';
	import { TouchableOpacity } from 'react-native-gesture-handler';			import { TouchableOpacity } from 'react-native-gesture-handler';

				import type { NavigationRoute } from '../navigation/route-names';
	import { useColors } from '../themes/colors.js';			import { useColors } from '../themes/colors.js';

	type Props = React.ElementConfig<typeof BaseHeaderBackButton>;			type ChangeRolesHeaderLeftButtonProps = {
	function ChangeRolesHeaderLeftButton(props: Props): React.Node {			+route: NavigationRoute<'ChangeRolesScreen'>,
				...React.ElementConfig<typeof BaseHeaderBackButton>,
				};

				function ChangeRolesHeaderLeftButton(
				props: ChangeRolesHeaderLeftButtonProps,
				): React.Node {
				const { memberInfo, role: selectedRole } = props.route.params;
				const { role: memberRole } = memberInfo;

				const onCancel = React.useCallback(() => {
				const { onPress } = props;
				invariant(onPress, 'onPress must be defined');

				if (selectedRole === memberRole) {
				onPress();
				return;
				}

				Alert.alert(
				'Unsaved Changes',
				'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
				[
				{ text: 'Cancel', onPress },
				{ text: 'Stay', style: 'cancel' },
				],
				{ cancelable: true },
				);
				}, [memberRole, props, selectedRole]);
				rohanAuthorUnsubmitted Done Inline Actions I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes. Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted) rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
				ashoatUnsubmitted Not Done Inline Actions What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
				rohanAuthorUnsubmitted Done Inline Actions Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues editing while `Discard edit` closes the alert and exits editing rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
				ashoatUnsubmitted Not Done Inline Actions I agree we shouldn't use the term `Cancel` here... how about `Leave`? I don't think we need `{ cancelable: true }`... we can just default to `Stay` if the user dismisses the `Alert` Can we try to match copy for title / description with the edit-message workflow? ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
				rohanAuthorUnsubmitted Done Inline Actions Yeah that makes sense, I'll update the diff rohan: Yeah that makes sense, I'll update the diff

	const { panelForegroundSecondaryLabel } = useColors();			const { panelForegroundSecondaryLabel } = useColors();
	const labelStyle = React.useMemo(			const labelStyle = React.useMemo(
	() => ({			() => ({
	color: panelForegroundSecondaryLabel,			color: panelForegroundSecondaryLabel,
	marginLeft: 10,			marginLeft: 10,
	}),			}),
	[panelForegroundSecondaryLabel],			[panelForegroundSecondaryLabel],
	);			);

	return (			return (
	<TouchableOpacity onPress={props.onPress}>			<TouchableOpacity onPress={onCancel}>
	<Text style={labelStyle}>Cancel</Text>			<Text style={labelStyle}>Cancel</Text>
	</TouchableOpacity>			</TouchableOpacity>
	);			);
	}			}

	export default ChangeRolesHeaderLeftButton;			export default ChangeRolesHeaderLeftButton;

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18397

web/markdown/markdown-spoiler.react.js

web/markdown/markdown.css

[web] Reveal the spoiler content on click within a messageClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18397

web/markdown/markdown-spoiler.react.js

web/markdown/markdown.css

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

Revision Contents
Changeset List