Details
- Go to any chat
- Open thread menu
- Open Create new subchannel
- Try to create any subchannel via ComposeSubchannel
For now, we don't have implemented announcement channels yet. In this case, when we try to create any type of announcement channels, this communiqué will appear:
In other cases, after successfully thread creation, we will be redirected to a newly created subchannel.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jakub/channel-composer
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/modals/threads/create/compose-subchannel-modal.react.js | ||
|---|---|---|
| 27 ↗ | (On Diff #16981) | matrix doesn't explain the purpose of this constant. Maybe threadTypesPerVisibility would be more descriptive? |
| 73 ↗ | (On Diff #16981) | I don't think it is necessary to create a state for loading. You should use createLoadingStatusSelector instead. |
| 97 ↗ | (On Diff #16981) | This is strange that we're setting loading to false only when there's an error. How the loading indicator behaves when a thread is created successfully? |
Adjust to code review:
- fix typo
- change matrix name to threadTypesPerVisibility
- use createLoadingStatusIndicator instead of using component state
| web/modals/threads/create/compose-subchannel-modal.react.js | ||
|---|---|---|
| 84 ↗ | (On Diff #16981) | It's an alternative for parseInt function. By default, announcement is boolean value thus when we try to use it as index in threadTypesMatrix in JavaScript, we need to convert it to int. Here is sample how it works:  |
| 97 ↗ | (On Diff #16981) | After successfully thread creation, modal disappear, so we don't need to return loading state to false. |
| web/modals/threads/create/compose-subchannel-modal.react.js | ||
|---|---|---|
| 34 ↗ | (On Diff #17198) | Maybe secret? |
| 84 ↗ | (On Diff #16981) | This is really fragile. JS conversions are terrible and relying on them is extremely risky. Also, it isn't obvious that the position in threadTypesPerVisibility matters and it's easy to break this. I think it would be better to have an object instead of an array and use announcement value in ternary expression. But still, this complication isn't necessary and I think the best solution here is to have a simple function that takes visibilityType and announcement and returns threadType as a result. (just like pendingThreadType in thread-utils does). |
| 97 ↗ | (On Diff #16981) | I don't see createLoadingStatusIndicator being used here. Is it in a different diff or this diff will be updated? |
Adjust to code review:
- change closed to secret
- replace threadTypesPerVisibility with getThreadType function
| web/modals/threads/create/compose-subchannel-modal.react.js | ||
|---|---|---|
| 97 ↗ | (On Diff #16981) | Yes, I used it in the latest diff update |
| web/modals/threads/create/compose-subchannel-modal.react.js | ||
|---|---|---|
| 107 ↗ | (On Diff #17239) | I think there are some other possible causes for this error and we shouldn't assume that this is due to announcement not being supported. So maybe e.message === 'invalid_parameters' && announcement might be a better condition? |
| 222 ↗ | (On Diff #17239) | Should the button be disabled also when loadingState === 'loading'? |