[web] Move `namePlaceholder` up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`
ClosedPublic
Actions

Authored by atul on Apr 18 2022, 7:46 PM.

Details

Reviewers

• jacek
tomek

Commits

Summary

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

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

Depends on D3760

Test Plan

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

atul requested review of this revision.Apr 18 2022, 7:52 PM

tomek requested changes to this revision.Apr 19 2022, 11:00 AM

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
444–449 ↗	(On Diff #11568)	We can't have an invariant here: we explicitly handle `!threadInfo` case in the next line, so it should be a big warning that something is really wrong. Another warning is that the original code didn't need an invariant - why was that? It looks like we should instead have a condition which returns `robotextName` or an empty / predefined string depending on the presence of `threadInfo`. It would be hard to catch this during testing, but looking at the message from the `if` it sounds like when a user has this component opened and is removed from it by someone else, we would call an invariant.

This revision now requires changes to proceed.Apr 19 2022, 11:00 AM

atul requested review of this revision.Apr 19 2022, 4:42 PM

atul added inline comments.

web/modals/threads/thread-settings-modal.react.js
444–449 ↗	(On Diff #11568)	Sorry, I should have explained this more clearly. This `invariant` is temporary and will be removed once `namePlaceholder` has been pushed down to the `ThreadSettingsGeneralTab` component. We can't move `namePlaceholder` below the early `!threadInfo` return because of the "rules of hooks," so it needs to be defined above that check. However, where `namePlaceholder` is defined right now it's possible for `threadInfo` to be `null/undefined`. But, because we know that this function will only ever be called from within the inner `ThreadSettingsModal`, we can be assured that `threadInfo` will be defined and use an `invariant`. In subsequent diffs this functionality will be pushed down to `ThreadSettingsGeneralTab` where we are assured that `threadInfo` is defined and can remove the `invariant`. Another warning is that the original code didn't need an invariant - why was that? The original code didn't need an invariant because it lived within the inner `ThreadSettingsModal` component where the `threadInfo` can't be `null/undefined`.

rebase

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
444–449 ↗	(On Diff #11568)	Thanks for the explanation! But, because we know that this function will only ever be called from within the inner ThreadSettingsModal, we can be assured that threadInfo will be defined and use an invariant. Ok, that makes sense. If that wasn't the case, I would prefer returning an empty string instead of invariant - just to don't break the existing code.