Paths

Table of Contentst

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

[web] Move `currentTabType` state up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`
ClosedPublic
Actions

Authored by atul on Apr 18 2022, 11:20 AM.

Details

Reviewers

• jacek
tomek

Commits

rCOMMc734ef12fd26: [web] Move `currentTabType` state up from `ThreadSettingsModal` to…

Summary

Move currentTabType state out of the inner class component state, and move it out to the wrapping "Connected" functional component.

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

Depends on D3757

Test Plan

Close reading... should be a noop

Will test the entirety of ThreadSettingsModal thoroughly once the refactor is complete (plan on using this as an opportunity to write some automated tests)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Apr 18 2022, 11:20 AM

Herald added subscribers: • abosh, • benschac, • karol and 2 others. · View Herald TranscriptApr 18 2022, 11:20 AM

Harbormaster completed remote builds in B8266: Diff 11557.Apr 18 2022, 11:25 AM

atul requested review of this revision.Apr 18 2022, 11:25 AM

atul added a child revision: D3760: [web] Move `queuedChanges` state up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.Apr 18 2022, 11:38 AM

atul edited the summary of this revision. (Show Details)Apr 18 2022, 10:02 PM

tomek accepted this revision.Apr 19 2022, 3:41 AM

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
393–395 ↗	(On Diff #11557)	This is probably fine, but we can consider some alternatives to calling an excessive number of functions: We can have one `useState` which would have an object instead of a separate state for each prop We can `useReducer` Not sure if it's worth changing - these are just ideas

This revision is now accepted and ready to land.Apr 19 2022, 3:41 AM

atul added inline comments.Apr 19 2022, 12:37 PM

web/modals/threads/thread-settings-modal.react.js
393–395 ↗	(On Diff #11557)	Definitely agree that there's room for cleaning things up here. Tried to keep the state "apart" for ease of refactoring, but once the state/functionality is pushed down to the relevant `ThreadSetting[General/Privacy/Delete]Modal`s I'll take another pass and see if there are things that can be consolidated/cleaned up/etc.

ashoat added inline comments.Apr 19 2022, 12:39 PM

web/modals/threads/thread-settings-modal.react.js
393–395 ↗	(On Diff #11557)	Personally I think it's probably fine

rebase before landing

Harbormaster completed remote builds in B8329: Diff 11637.Apr 19 2022, 12:54 PM

Closed by commit rCOMMc734ef12fd26: [web] Move `currentTabType` state up from `ThreadSettingsModal` to… (authored by atul). · Explain WhyApr 19 2022, 12:56 PM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMc734ef12fd26: [web] Move `currentTabType` state up from `ThreadSettingsModal` to….

Revision Contents
Changeset List

Path

Size

web/

modals/

threads/

thread-settings-modal.react.js

31 lines

Diff 11638

View Options

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

	Show All 21 Lines
	a {			a {
	text-decoration: none;			text-decoration: none;
	color: #2a5db0;			color: #2a5db0;
	}			}
	img,			img,
	iframe {			iframe {
	display: block;			display: block;
	}			}
	input[type='text'],			input[type='text'],
	input[type='password'],			input[type='password'],
	textarea {			textarea {
	atulUnsubmitted Not Done Inline Actions Moving the global input styling and moving it to the input component. Then why are we keeping styles like `border` and `border-radius`? Don't these styles also affect the `<textarea>` tag? Are you sure we want to be removing these styles for that as well? Or should we break this into two separate selectors (one for `input` and another for `textarea`)? atul: > Moving the global input styling and moving it to the input component. Then why are we…
	benschacAuthorUnsubmitted Done Inline Actions Yeah, I should break these styles out a bit more. I don't think they really do anything. But you make a good point that this will also effect `textarea` elements. benschac: Yeah, I should break these styles out a bit more. I don't think they really do anything. But…
	-webkit-appearance: none;
	-moz-appearance: none;
	-webkit-border-radius: 0;
	border: 1px solid #dddddd;			border: 1px solid #dddddd;
	border-radius: 1px;			border-radius: 1px;
	font-family: var(--font-stack);			font-family: var(--font-stack);
	}			}
	:global(#react-root) {			:global(#react-root) {
	display: flex;			display: flex;
	flex-direction: column;			flex-direction: column;
	height: 100%;			height: 100%;
	▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines

[web] Move `currentTabType` state up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`ClosedPublicActions