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
Details
Details
- Opened ThreadSettingsModal for a thread
- Ensured that the placeholder was as expected
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- landapril20 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| 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. |
| 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.
The original code didn't need an invariant because it lived within the inner ThreadSettingsModal component where the threadInfo can't be null/undefined. |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 444–449 ↗ | (On Diff #11568) | Thanks for the explanation!
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. |
Comment Actions
If that wasn't the case, I would prefer returning an empty string instead of invariant
Yeah I agree, this will be moved to the ThreadSettingsGeneralTab soon at which point we'll be able to cut the invariant.