Details

Reviewers

• jacek
tomek

Commits

Summary

Move onChangeName out from inner class component to outer "Connected" functional component.

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

Depends on D3764

Test Plan

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

atul requested review of this revision.Apr 18 2022, 2:39 PM

web/modals/threads/thread-settings-modal.react.js
438 ↗	(On Diff #11574)	Is this invariant for testing purposes? It looks like we handle the case when `threadInfo` is `null` or `undefined` on line 450.

web/modals/threads/thread-settings-modal.react.js
438 ↗	(On Diff #11574)	`invariant` is always for Flow. @atul is using it here so that Flow won't complain when he dereferences `threadInfo.name`. Flow doesn't realize that there's no way for `onChangeName` to be called when `threadInfo` is falsey, so @atul explicitly annotates this to Flow using `invariant`

rebase

tomek requested changes to this revision.Apr 21 2022, 3:04 AM

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
444–450 ↗	(On Diff #11646)	Do we really need an invariant here? When `threadInfo.name` is not present, we have undefined... maybe we can do the same when `threadInfo` is not present? Additional benefit would be that the callback will depend on `threadInfo?.name` instead of the whole `threadInfo`.

This revision now requires changes to proceed.Apr 21 2022, 3:04 AM

atul planned changes to this revision.Apr 21 2022, 9:09 AM

atul added inline comments.

web/modals/threads/thread-settings-modal.react.js
444–450 ↗	(On Diff #11646)	I think if the `onChangeName` callback were to stay here it would make sense, but it's going to be pushed down to the `ThreadSettingsGeneral` tab where existence of `threadInfo` is guaranteed. With either the invariant or the optional chaining we'll need to revert after pushing the functionality down, and the invariant seems like an easier thing to find and revert? Still open to making the change to optional chaining though

atul requested review of this revision.Apr 21 2022, 9:10 AM

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
444–450 ↗	(On Diff #11646)	Ok, so up to you. My preference is to avoid invariants and use them only when there's no other way. We're going to move this code, so when pasting, we will read it and verify that it's correct - so searching shouldn't be needed, I guess. But as I said, up to you. If you prefer keeping the invariant, we can have `threadInfo?.name` in it, so that the dependencies are as narrow as possible.