[web] Lift `buttons: React.Node` from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`
ClosedPublic
Actions

Authored by atul on Apr 24 2022, 12:26 PM.

Details

Reviewers

• jacek
tomek

Commits

rCOMMe9c961bedef6: [web] Lift `buttons: React.Node` from `ThreadSettingsModal` to…

Summary

Lift button from the ThreadSettingsModal class component to the ConnectedThreadSettingsModal functional component.

Doing this to "stage" the lifting of JSX markup into separate steps to hopefully make it easier to review.

As part of the work to refactor ThreadSettingsModal... specifically to turn it into a functional component.

Depends on D3822

Test Plan

Open ThreadSettingsModal
Make sure the "buttons" of the ThreadSettingsModal component continues to look and behave as expected (since that was what was moved)
Change some settings around, hit save, make sure things are saved as expected

Diff Detail

Repository

rCOMM Comm

Branch

clean (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 24 2022, 12:26 PM

Herald added subscribers: • abosh, • karol, • adrian, ashoat. · View Herald TranscriptApr 24 2022, 12:26 PM

Harbormaster completed remote builds in B8482: Diff 11827.Apr 24 2022, 12:30 PM

atul requested review of this revision.Apr 24 2022, 12:30 PM

fix

atul added a child revision: D3824: [web] Lift `tabs: React.Node` from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.Apr 24 2022, 12:45 PM

Harbormaster completed remote builds in B8483: Diff 11828.Apr 24 2022, 12:45 PM

tomek accepted this revision.Apr 26 2022, 7:58 AM

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
109 ↗	(On Diff #11828)	It seems like this is always defined. Can we avoid using optional here?
453–471 ↗	(On Diff #11828)	What do you think about wrapping it with memo?

This revision is now accepted and ready to land.Apr 26 2022, 7:58 AM

rebase

Harbormaster completed remote builds in B8564: Diff 11943.Apr 26 2022, 10:14 AM

atul added inline comments.Apr 26 2022, 10:18 AM

web/modals/threads/thread-settings-modal.react.js
109 ↗	(On Diff #11828)	Flow wasn't able to determine that `button` is always defined so I made this `React.Node` prop optional for now. The class component gets cut entirely in the diff after this one (D3825) so I didn't spend too much time trying to do this "correctly."
453–471 ↗	(On Diff #11828)	That definitely makes sense. Just moving things around in this diff, the buttons eventually move into the `ThreadSettings[General/Privacy/Delete]Tabs` child components in later diffs. I was planning on doing three "passes" after the refactor to make sure everything is good since a lot of things changed/got moved around. Optimizing (avoiding unnecessary renders, memoizing things, etc.) Styling (making sure that things look consistent, styles aren't broken, etc) Testing (go through all of the `ThreadSettingsModal` functionality thoroughly to ensure that there aren't any regressions) Will take a look at memoizing the buttons in the first pass.

atul mentioned this in D3824: [web] Lift `tabs: React.Node` from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.Apr 26 2022, 10:22 AM

rebase before landing

Harbormaster completed remote builds in B8580: Diff 11959.Apr 26 2022, 12:07 PM

Closed by commit rCOMMe9c961bedef6: [web] Lift `buttons: React.Node` from `ThreadSettingsModal` to… (authored by atul). · Explain WhyApr 26 2022, 12:10 PM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMe9c961bedef6: [web] Lift `buttons: React.Node` from `ThreadSettingsModal` to….

atul mentioned this in D3845: [web] Use new `Tabs.Container` instead of existing `Tabs` component.Apr 28 2022, 3:12 PM

Revision Contents
Changeset List

Path

Size

web/

modals/

threads/

thread-settings-modal.react.js

51 lines

Diff 11943

View Options

web/modals/threads/thread-settings-modal.react.js

Show First 20 Lines • Show All 89 Lines • ▼ Show 20 Lines	type Props = {
+accountPassword: string,		+accountPassword: string,
+setAccountPassword: SetState<string>,		+setAccountPassword: SetState<string>,
+currentTabType: TabType,		+currentTabType: TabType,
+setCurrentTabType: SetState<TabType>,		+setCurrentTabType: SetState<TabType>,
+queuedChanges: ThreadChanges,		+queuedChanges: ThreadChanges,
+setQueuedChanges: SetState<ThreadChanges>,		+setQueuedChanges: SetState<ThreadChanges>,
+namePlaceholder: string,		+namePlaceholder: string,
+changeQueued: boolean,		+changeQueued: boolean,
		+onChangeName: (event: SyntheticEvent<HTMLInputElement>) => void,
};		};
class ThreadSettingsModal extends React.PureComponent<Props> {		class ThreadSettingsModal extends React.PureComponent<Props> {
nameInput: ?HTMLInputElement;		nameInput: ?HTMLInputElement;
accountPasswordInput: ?HTMLInputElement;		accountPasswordInput: ?HTMLInputElement;

constructor(props: Props) {		constructor(props: Props) {
super(props);		super(props);
}		}
▲ Show 20 Lines • Show All 55 Lines • ▼ Show 20 Lines	const inputDisabled =
!this.hasPermissionForTab(threadInfo, this.props.currentTabType);		!this.hasPermissionForTab(threadInfo, this.props.currentTabType);

let mainContent = null;		let mainContent = null;
if (this.props.currentTabType === 'general') {		if (this.props.currentTabType === 'general') {
mainContent = (		mainContent = (
<ThreadSettingsGeneralTab		<ThreadSettingsGeneralTab
threadNameValue={firstLine(this.possiblyChangedValue('name'))}		threadNameValue={firstLine(this.possiblyChangedValue('name'))}
threadNamePlaceholder={this.props.namePlaceholder}		threadNamePlaceholder={this.props.namePlaceholder}
threadNameOnChange={this.onChangeName}		threadNameOnChange={this.props.onChangeName}
threadNameDisabled={inputDisabled}		threadNameDisabled={inputDisabled}
threadNameInputRef={this.nameInputRef}		threadNameInputRef={this.nameInputRef}
threadDescriptionValue={this.possiblyChangedValue('description')}		threadDescriptionValue={this.possiblyChangedValue('description')}
threadDescriptionOnChange={this.onChangeDescription}		threadDescriptionOnChange={this.onChangeDescription}
threadDescriptionDisabled={inputDisabled}		threadDescriptionDisabled={inputDisabled}
threadColorCurrentColor={this.possiblyChangedValue('color')}		threadColorCurrentColor={this.possiblyChangedValue('color')}
threadColorOnColorSelection={this.onChangeColor}		threadColorOnColorSelection={this.onChangeColor}
/>		/>
▲ Show 20 Lines • Show All 110 Lines • ▼ Show 20 Lines	class ThreadSettingsModal extends React.PureComponent<Props> {
nameInputRef = (nameInput: ?HTMLInputElement) => {		nameInputRef = (nameInput: ?HTMLInputElement) => {
this.nameInput = nameInput;		this.nameInput = nameInput;
};		};

accountPasswordInputRef = (accountPasswordInput: ?HTMLInputElement) => {		accountPasswordInputRef = (accountPasswordInput: ?HTMLInputElement) => {
this.accountPasswordInput = accountPasswordInput;		this.accountPasswordInput = accountPasswordInput;
};		};

onChangeName = (event: SyntheticEvent<HTMLInputElement>) => {
const target = event.currentTarget;
const newValue =
target.value !== this.props.threadInfo.name ? target.value : undefined;
this.props.setQueuedChanges(
Object.freeze({
...this.props.queuedChanges,
name: firstLine(newValue),
}),
);
};

onChangeDescription = (event: SyntheticEvent<HTMLTextAreaElement>) => {		onChangeDescription = (event: SyntheticEvent<HTMLTextAreaElement>) => {
const target = event.currentTarget;		const target = event.currentTarget;
const newValue =		const newValue =
target.value !== this.props.threadInfo.description		target.value !== this.props.threadInfo.description
? target.value		? target.value
: undefined;		: undefined;
this.props.setQueuedChanges(		this.props.setQueuedChanges(
Object.freeze({		Object.freeze({
▲ Show 20 Lines • Show All 127 Lines • ▼ Show 20 Lines	function ConnectedThreadSettingsModal(props) {
}, [threadInfo, userInfos, viewerID]);		}, [threadInfo, userInfos, viewerID]);

const changeQueued: boolean = React.useMemo(		const changeQueued: boolean = React.useMemo(
() =>		() =>
Object.values(queuedChanges).some(v => v !== null && v !== undefined),		Object.values(queuedChanges).some(v => v !== null && v !== undefined),
[queuedChanges],		[queuedChanges],
);		);

		const onChangeName = React.useCallback(
		(event: SyntheticEvent<HTMLInputElement>) => {
		const target = event.currentTarget;
		setQueuedChanges(
		Object.freeze({
		...queuedChanges,
		name: firstLine(
		target.value !== threadInfo?.name ? target.value : undefined,
		),
		}),
		);
		},
		[queuedChanges, threadInfo],
		);

if (!threadInfo) {		if (!threadInfo) {
return (		return (
<Modal onClose={modalContext.clearModal} name="Invalid thread">		<Modal onClose={modalContext.clearModal} name="Invalid thread">
<div className={css.modal_body}>		<div className={css.modal_body}>
<p>You no longer have permission to view this thread</p>		<p>You no longer have permission to view this thread</p>
</div>		</div>
</Modal>		</Modal>
);		);
Show All 15 Lines	return (
accountPassword={accountPassword}		accountPassword={accountPassword}
setAccountPassword={setAccountPassword}		setAccountPassword={setAccountPassword}
currentTabType={currentTabType}		currentTabType={currentTabType}
setCurrentTabType={setCurrentTabType}		setCurrentTabType={setCurrentTabType}
queuedChanges={queuedChanges}		queuedChanges={queuedChanges}
setQueuedChanges={setQueuedChanges}		setQueuedChanges={setQueuedChanges}
namePlaceholder={namePlaceholder}		namePlaceholder={namePlaceholder}
changeQueued={changeQueued}		changeQueued={changeQueued}
		onChangeName={onChangeName}
/>		/>
);		);
},		},
);		);

export default ConnectedThreadSettingsModal;		export default ConnectedThreadSettingsModal;

[web] Lift `buttons: React.Node` from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`ClosedPublicActions