Paths

Table of Contentst

-
web/modals/threads/settings/
-
modals/
-
threads/
-
settings/
2/5
thread-settings-general-tab.react.js
2/5
thread-settings-modal.react.js

Differential D4073

[web] Fix thread description and color modification in private thread
ClosedPublic
Actions

Authored by • abosh on May 18 2022, 10:07 PM.

Details

Reviewers

atul

Commits

rCOMM49e364a933bc: [web] Fix thread description and color modification in private thread

Summary

Fix issue where user is unable to modify the thread description and color in their private thread. The thread name is still unmodifiable (following the inclusion of the EDIT_DESCRIPTION and EDIT_COLOR permissions but not the EDIT_THREAD_NAME permission in D1980), but the thread description and color can be modified.

Related Linear issue with full context here.

Test Plan

Tested on Chrome/Safari and works as expected:

Before:

After:

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• abosh created this revision.May 18 2022, 10:07 PM

Herald added subscribers: • adrian, tomek, ashoat. · View Herald TranscriptMay 18 2022, 10:07 PM

• abosh edited the summary of this revision. (Show Details)May 18 2022, 10:11 PM

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

• abosh added 1 blocking reviewer(s): atul.

Harbormaster completed remote builds in B9316: Diff 12917.May 18 2022, 10:12 PM

• abosh requested review of this revision.May 18 2022, 10:12 PM

• abosh edited the test plan for this revision. (Show Details)May 18 2022, 10:13 PM

• abosh added inline comments.May 18 2022, 10:16 PM

web/modals/threads/settings/thread-settings-general-tab.react.js
149–152 ↗	(On Diff #12917)	This extra condition makes sure that the thread has the `EDIT_THREAD_NAME` permission. Private threads do not have this permission, so this condition will ensure that the `Input` will be disabled if the thread is a private thread.

• abosh edited the summary of this revision. (Show Details)May 18 2022, 10:18 PM

atul requested changes to this revision.May 19 2022, 6:18 AM

atul added inline comments.

web/modals/threads/settings/thread-settings-general-tab.react.js
149–152 ↗	(On Diff #12917)	Looks good, can we pull this conditional outside of the `disabled` prop? Maybe introduce an `threadNameInputDisabled` variable that gets passed to the `disabled` prop?

This revision now requires changes to proceed.May 19 2022, 6:18 AM

Moved conditional outside disabled prop and memoized the call to threadHasPermission so it's only called once the dependencies changed and not each time the Input is rendered.

Harbormaster completed remote builds in B9361: Diff 12973.May 20 2022, 4:22 AM

Let's "un-memoize" threadNameInputDisabled and then this should be good to land.

web/modals/threads/settings/thread-settings-general-tab.react.js
136–139 ↗	(On Diff #12973)	We don't need to memoize this since `threadHasPermission` returns a boolean. We typically use `React.useMemo` when we're getting an object (eg array, function, etc) and want to preserve referential equality. Might also make sense to memoize something that's computationally expensive, but I don't think `threadHasPermission` is?

This revision now requires changes to proceed.May 20 2022, 7:50 AM

Got it. I assumed we needed to memoize it since I was only looking at the dependency (threadInfo) but didn't realize threadNameInputDisabled only returns a boolean which is much less expensive to compute in this case.

Harbormaster completed remote builds in B9371: Diff 12987.May 20 2022, 12:03 PM

atul accepted this revision.May 20 2022, 12:04 PM

atul added inline comments.

web/modals/threads/settings/thread-settings-general-tab.react.js
152	We should probably rename this in another diff to something like `threadSettingsOperationInProgress` or something to make it more descriptive.

This revision is now accepted and ready to land.May 20 2022, 12:04 PM

Closed by commit rCOMM49e364a933bc: [web] Fix thread description and color modification in private thread (authored by • abosh). · Explain WhyMay 20 2022, 12:15 PM

This revision was automatically updated to reflect the committed changes.

• abosh added a commit: rCOMM49e364a933bc: [web] Fix thread description and color modification in private thread.

• abosh mentioned this in D4105: [web] Rename `inputDisabled` to `threadSettingsOperationInProgress` in `ThreadSettingsTab`s.Jun 17 2022, 1:16 PM

Revision Contents
Changeset List

Path

Size

web/

modals/

threads/

settings/

thread-settings-general-tab.react.js

14 lines

thread-settings-modal.react.js

10 lines

Diff 12987

View Options

web/modals/threads/settings/thread-settings-general-tab.react.js

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
import {		import {
RelationshipListItemTooltipModalRouteName,		RelationshipListItemTooltipModalRouteName,
FriendListRouteName,		FriendListRouteName,
BlockListRouteName,		BlockListRouteName,
} from '../navigation/route-names.js';		} from '../navigation/route-names.js';
import { useSelector } from '../redux/redux-utils.js';		import { useSelector } from '../redux/redux-utils.js';
import { type Colors, useColors, useStyles } from '../themes/colors.js';		import { type Colors, useColors, useStyles } from '../themes/colors.js';
import type { VerticalBounds } from '../types/layout-types.js';		import type { VerticalBounds } from '../types/layout-types.js';
		import { useNavigateToUserProfileBottomSheet } from '../user-profile/user-profile-utils.js';
import Alert from '../utils/alert.js';		import Alert from '../utils/alert.js';

type BaseProps = {		type BaseProps = {
+userInfo: AccountUserInfo,		+userInfo: AccountUserInfo,
+lastListItem: boolean,		+lastListItem: boolean,
+verticalBounds: ?VerticalBounds,		+verticalBounds: ?VerticalBounds,
+relationshipListRoute: NavigationRoute<'FriendList' \| 'BlockList'>,		+relationshipListRoute: NavigationRoute<'FriendList' \| 'BlockList'>,
+navigate: RelationshipListNavigate,		+navigate: RelationshipListNavigate,
Show All 10 Lines	type Props = {
// async functions that hit server APIs		// async functions that hit server APIs
+updateRelationships: (		+updateRelationships: (
request: RelationshipRequest,		request: RelationshipRequest,
) => Promise<RelationshipErrors>,		) => Promise<RelationshipErrors>,
// withOverlayContext		// withOverlayContext
+overlayContext: ?OverlayContextType,		+overlayContext: ?OverlayContextType,
// withKeyboardState		// withKeyboardState
+keyboardState: ?KeyboardState,		+keyboardState: ?KeyboardState,
		+navigateToUserProfileBottomSheet: (userID: string) => mixed,
};		};
class RelationshipListItem extends React.PureComponent<Props> {		class RelationshipListItem extends React.PureComponent<Props> {
editButton = React.createRef<React.ElementRef<typeof View>>();		editButton = React.createRef<React.ElementRef<typeof View>>();

render() {		render() {
const {		const {
lastListItem,		lastListItem,
removeUserLoadingStatus,		removeUserLoadingStatus,
▲ Show 20 Lines • Show All 77 Lines • ▼ Show 20 Lines	) {
>		>
<Text style={this.props.styles.blueAction}>Add</Text>		<Text style={this.props.styles.blueAction}>Add</Text>
</TouchableOpacity>		</TouchableOpacity>
);		);
}		}

return (		return (
<View style={[this.props.styles.container, borderBottom]}>		<View style={[this.props.styles.container, borderBottom]}>
		<TouchableOpacity
		ginsuAuthorUnsubmitted Done Inline Actions The red background shows the `TouchableOpacity`: I think it would be a be a better user experience if we had the entire item be a touchable instead of just the avatar/username ginsu: The red background shows the `TouchableOpacity`: {F773403} I think it would be a be a better…
		atulUnsubmitted Not Done Inline Actions What surfaces should be "touchable" is probably something we should consider as part of the Design System (at some point) CC @ashoat @ted In this particular case, it feels a little weird that we're extending so far to the right, but on the left we go just until the left edge. I guess this is a new scenario, because previously we were just listing "static" avatars/usernames and left all the interaction to the right-justified icon. Unsolicited, but if it were up to me I think it'd make sense to give the avatar/username some sort of inset to make it clear that A. the element is interactive B. the bounds of the tap target Something like: or atul: What surfaces should be "touchable" is probably something we should consider as part of the…
		ashoatUnsubmitted Not Done Inline Actions I like @atul's idea. Curious for @ted's thoughts, both on Atul's idea and also on how we can include touchable surface / interaction design of touchables in the design system ashoat: I like @atul's idea. Curious for @ted's thoughts, both on Atul's idea and also on how we can…
		tedUnsubmitted Not Done Inline Actions In this particular case, it feels a little weird that we're extending so far to the right, but on the left we go just until the left edge. I think the touch point can extend more on the left, to where `Search` starts at the input field above. @atul @ashoat I think adding touch points to the design system makes perfect sense and can be included for both web and mobile components. Each design system component lives on its own page in Figma, which will also include small documents to specifically point out the interactive touch points. These documents can point out other component rules such as places to use this component, certain rules of how to use this component, etc. For the concepts you shared, I don't think that adding an inset is necessary to point out that the element is interactive. Adding an inset feels a bit cluttered to the overall list layout. If we do want to use an inset container for the avatar and username, then reducing the separator lines (smaller, more subtle) will help with over-cluttering. I took a quick look at Discord's username touchpoints in their list. In the video, I am tapping as far right as I can before the two action buttons. The entire width seems tappable except for the two buttons. Not saying we need to do exactly how Discord handles this interaction, I wanted to see how other apps were handling this. With that, I think the UI is fine how it is, but always open if more people think we should add an inset or interactive indicator. Thank you! ted: > In this particular case, it feels a little weird that we're extending so far to the right…
		ginsuAuthorUnsubmitted Done Inline Actions For the concepts you shared, I don't think that adding an inset is necessary to point out that the element is interactive. Adding an inset feels a bit cluttered to the overall list layout. If we do want to use an inset container for the avatar and username, then reducing the separator lines (smaller, more subtle) will help with over-cluttering. I agree with this point, I think especially since we have the edit (pencil) icon to the right it would feel strange if that didn't have an inset/obvious indicator that the icon is touchable but the avatar/username does. To polish this up, I can make the entire container a touchable or at the very least, also extend the touchable to be touching the left side of this container ginsu: > For the concepts you shared, I don't think that adding an inset is necessary to point out…
		onPress={this.onPressUser}
		style={this.props.styles.userInfoContainer}
		>
<UserAvatar size="small" userID={this.props.userInfo.id} />		<UserAvatar size="small" userID={this.props.userInfo.id} />
<SingleLine style={this.props.styles.username}>		<SingleLine style={this.props.styles.username}>
{this.props.userInfo.username}		{this.props.userInfo.username}
</SingleLine>		</SingleLine>
		</TouchableOpacity>
{editButton}		{editButton}
</View>		</View>
);		);
}		}

		onPressUser = () => {
		this.props.navigateToUserProfileBottomSheet(this.props.userInfo.id);
		};

onSelect = () => {		onSelect = () => {
const { id, username } = this.props.userInfo;		const { id, username } = this.props.userInfo;
this.props.onSelect({ id, username });		this.props.onSelect({ id, username });
};		};

visibleEntryIDs() {		visibleEntryIDs() {
const { relationshipListRoute } = this.props;		const { relationshipListRoute } = this.props;
const id = {		const id = {
▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Lines	container: {
paddingHorizontal: 24,		paddingHorizontal: 24,
paddingVertical: 10,		paddingVertical: 10,
backgroundColor: 'panelForeground',		backgroundColor: 'panelForeground',
borderColor: 'panelForegroundBorder',		borderColor: 'panelForegroundBorder',
},		},
borderBottom: {		borderBottom: {
borderBottomWidth: 1,		borderBottomWidth: 1,
},		},
		userInfoContainer: {
		flex: 1,
		flexDirection: 'row',
		},
buttonContainer: {		buttonContainer: {
flexDirection: 'row',		flexDirection: 'row',
},		},
editButtonWithMargin: {		editButtonWithMargin: {
marginLeft: 15,		marginLeft: 15,
},		},
username: {		username: {
color: 'panelForegroundSecondaryLabel',		color: 'panelForegroundSecondaryLabel',
Show All 29 Lines	) {
);		);
const colors = useColors();		const colors = useColors();
const styles = useStyles(unboundStyles);		const styles = useStyles(unboundStyles);
const dispatchActionPromise = useDispatchActionPromise();		const dispatchActionPromise = useDispatchActionPromise();
const boundUpdateRelationships = useServerCall(updateRelationships);		const boundUpdateRelationships = useServerCall(updateRelationships);
const overlayContext = React.useContext(OverlayContext);		const overlayContext = React.useContext(OverlayContext);
const keyboardState = React.useContext(KeyboardContext);		const keyboardState = React.useContext(KeyboardContext);

		const navigateToUserProfileBottomSheet =
		useNavigateToUserProfileBottomSheet();

return (		return (
<RelationshipListItem		<RelationshipListItem
{...props}		{...props}
removeUserLoadingStatus={removeUserLoadingStatus}		removeUserLoadingStatus={removeUserLoadingStatus}
colors={colors}		colors={colors}
styles={styles}		styles={styles}
dispatchActionPromise={dispatchActionPromise}		dispatchActionPromise={dispatchActionPromise}
updateRelationships={boundUpdateRelationships}		updateRelationships={boundUpdateRelationships}
overlayContext={overlayContext}		overlayContext={overlayContext}
keyboardState={keyboardState}		keyboardState={keyboardState}
		navigateToUserProfileBottomSheet={navigateToUserProfileBottomSheet}
/>		/>
);		);
});		});

export default ConnectedRelationshipListItem;		export default ConnectedRelationshipListItem;