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

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

Details

Reviewers

• jacek
tomek

Commits

Summary

Lift tabs 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 D3823

Test Plan

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

Repository

rCOMM Comm

Branch

clean (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

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

web/modals/threads/thread-settings-modal.react.js
416–459 ↗	(On Diff #11829)	Should this be memoized? (Open to arguments against)

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
110 ↗	(On Diff #11829)	Do we need to use optional?
416–459 ↗	(On Diff #11829)	I had a similar question in the previous diff. I think that memoizing here makes sense - it's more predictable when props change if and only if the content is different.

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

rebase

web/modals/threads/thread-settings-modal.react.js
110 ↗	(On Diff #11829)	see https://phabricator.ashoat.com/D3823?id=11828#inline-23419
416–459 ↗	(On Diff #11829)	The goal of this diff was to just move the tabs out from the class component to the outer functional component. We stop using this approach to `Tabs` entirely and move to the `Tabs.Container`/`Tabs.Item` components introduced by @def-au1t in D3845, so this gets ripped out entirely. However, the tabs are memoized with that new approach.