[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

Branch

apr19 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

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.