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
Details
Details
- Open ThreadSettingsModal
- Change the thread name a bunch
- Log values in onChangeName to ensure that they continue to be as expected
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| 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) | |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 444–450 | 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. | |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 444–450 | 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 | |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 444–450 | 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. | |