Details
This modal should pop up when deleting:
- Non-empty community
- Subchannel with threads
Modal should not pop up when deleting:
- Empty community
- Empty subchannel
- Thread
Note: right now we don't support deleting subchannels when the parent channel is deleted. What I mean is this situation:
-> Community
--> Subchannel 1
---> Subchannel 2
If we delete Subchannel 1, right now Subchannel 2 is not deleted, thus modal should not pop up.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- patryk/web-warn-user
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/modals/threads/settings/thread-settings-delete-tab.react.js | ||
|---|---|---|
| 79 | I wanted to use the finally block to avoid repeating this function call in line 75. However, in doing so, we may experience a delay in closing the modal when the callDeleteThread function is successful | |
| web/modals/threads/settings/thread-settings-delete-confirmation-modal.react.js | ||
|---|---|---|
| 12 | I don't think we call community chats "channel roots". Maybe just isCommunity? | |
| web/modals/threads/settings/thread-settings-delete-tab.react.js | ||
| 55–60 | You missed GENESIS. GENESIS is a special channel type in our codebase, that allows us to do hacks. It is a community root. | |
| 79 | Isn't the delay still possible since you call popThreadDeleteConfirmationModal after await callDeleteThread? | |
| web/modals/threads/settings/thread-settings-delete-confirmation-modal.react.js | ||
|---|---|---|
| 36 ↗ | (On Diff #28890) | If it's a community root, we should say "within this community" instead of "within this chat" |
| web/modals/threads/settings/thread-settings-delete-confirmation-modal.react.js | ||
|---|---|---|
| 36 ↗ | (On Diff #28890) | We could consider using threadNoun instead of "chat" here, but it doesn't seem to handle communities very well We could introduce a new function that uses "community" if threadTypeIsCommunityRoot, and otherwise calls threadNoun |
| web/modals/threads/settings/thread-settings-delete-tab.react.js | ||
| 60 ↗ | (On Diff #28890) | One thing to consider here... in the future, we may change it, and make it so we don't always have threads available in the Redux store, even if they exist Right now, the Redux store is very large and very expensive in terms of performance If we remove old threads from the Redux store, then this check will no longer be viable Before landing, can you add a subtask to ENG-4423 to address this in the future? |
| web/modals/threads/settings/thread-settings-delete-confirmation-modal.react.js | ||
|---|---|---|
| 36 ↗ | (On Diff #28936) | Doesn't it now say "community" for normal chats (not communities) as well?? |
| web/modals/threads/settings/thread-settings-delete-confirmation-modal.react.js | ||
|---|---|---|
| 36 ↗ | (On Diff #28936) | Yes, I think my feedback was misinterpreted. Here's a very precise list of instructions:
|
Introduce a new function
Please introduce a new function in lib/shared/thread-utils.js, so you can reuse it on native. My feedback was extremely specific... please try rereading it closely and slowly.
Introduce a new function that calls return "community" if threadTypeIsCommunityRoot, and
threadNoun otherwise and use it in threadsToDeleteText variable.
Thank you!!
| web/modals/threads/settings/thread-settings-delete-confirmation-modal.react.js | ||
|---|---|---|
| 16 ↗ | (On Diff #29179) | If you are taking a function as input, and you don't care what that function returns, it is better to use () => mixed. That is a more flexible contract, and allows your caller to eg. hand you an async function (which returns a Promise) without having to wrap it in a lambda |