Details

Reviewers

• jacek
tomek

Commits

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

fix

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

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.