Move deleteThreadAction 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 D3770
Details
Details
- Open ThreadSettingsModal
- Go to Delete tab
- Enter password and delete thread
- Ensure that things look/behave as expected
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 465–466 ↗ | (On Diff #11652) | Yes, I know we consider leaving TODO comments in the code an anti-pattern, but I think it's the best way forward in this case. The "ref" is currently "within" the class component and can't be accessed from the outer functional component. "Why not pull the input refs out from the class component first then?" I tried sequencing the "lifting of refs" before this and found it very frustrating... particularly with regard to flow issues. I think the best bet is to leave a reminder here that we want to focus that input, and then once we've "turned ThreadSettingsModal into a functional component," we can introduce replacement refs with React.useRef and bring back this focus behavior. |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 456 ↗ | (On Diff #11652) | @def-au1t in his stack introduces a new method popModal that should be used here. The method clearModal will not be defined after that stack so there's no risk of forgetting about the update here. |
| 465–466 ↗ | (On Diff #11652) | Thanks for the explanation - really like the fact that it explains what's going on and why. Do you think it is a good idea to create a linear task for it? |
| 469 ↗ | (On Diff #11652) | Can we modify the code so that the callback depends only on thread.id? |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 456 ↗ | (On Diff #11652) | Ah thanks for the heads up. It looks that stack requires revision before it can be landed. I think the best bet is for me to
That way I should be able to move forward with the ThreadSettingsModal refactor without breaking Jacek's existing work? cc @def-au1t |
| 465–466 ↗ | (On Diff #11652) |
Sure, don't think it would hurt: https://linear.app/comm/issue/ENG-1028/[web]-reintroduce-focusing-of-inputs-in-threadsettingsmodal |
| 469 ↗ | (On Diff #11652) | Yup, will make that change |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 469 ↗ | (On Diff #11652) | Hm, when I modify the deplist I get the following:
Going to land as is, open to making any changes in a subsequent diff if necessary |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 456 ↗ | (On Diff #11652) | Up to you, but usually the diff author should be responsible for rebasing and resolving conflicts. It's a part of the process that we're introducing conflicting changes. |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 469 ↗ | (On Diff #11652) | That's due to invariant using threadInfo. We can change that and use invariant(threadInfo.id, ... or, if that doesn't work, introduce a new variable threadID before the callback and use it as a dependency and in the invariant. Overall, it is a bit wasteful to create a new callback every time a threadInfo changes, when we only use id part of it. This improvement should be introduced, but may be easier when invariants are gone. |
| web/modals/threads/thread-settings-modal.react.js | ||
|---|---|---|
| 469 ↗ | (On Diff #11652) | Sometimes that happens, in which case you can just pull threadInfo.id out into its own variable outside of the block |