Excluded ChatTopBar and ChatInputBar components and thread selection logic from ChatMessageList into ChatMessageListContainer. This approach follows the solution in native app.
The MessageList component will not contain logic for selecting threads and the change will separate logic responsible for displaying thread content from thread selection logic (which will be extended in diffs introducing thread creation).
There are no changes in code logic - only moving and changes related to types.
Details
Details
Running web app and confirming it works aas before. Confirm if drag and drop for files still works.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 376 ↗ | (On Diff #14131) | The variable was incorrectly typed, and the type here should be narrowed to boolean as it's in component definition. The previous type was wrong but worked here because of any-typed dndProps when passing parameters into component (we discussed it with @palys-swm) |
| 377 ↗ | (On Diff #14131) | The ID should depend directly on threadInfo passed to component instead of value received from navigation. As these ID values should be the same (as for now), it was not a problem but should be changed to avoid problems when introducing navigation for thread creation (in next diffs) |
Comment Actions
Couple of minor questions inline
| web/chat/chat-message-list-container.css | ||
|---|---|---|
| 10 ↗ | (On Diff #14131) | Saw that this was copy/pasted from existing CSS, but we should probably move it to theme.css and refer to it here as var(--blah) |
| web/chat/chat-message-list-container.react.js | ||
| 23–32 ↗ | (On Diff #14131) | Do we need to a create a new variable activeID that's set to activeChatThreadID? Or can we just use activeChatThreadID directly? (Noticed that this was originally in ChatMessageList, so realize you didn't necessarily choose to do it like this) |
| 82 ↗ | (On Diff #14131) | Do we want to keep this console.log around, or was it just included to help with developing/debugging? If we want to keep it around, we should probably have a more descriptive log message... but my guess is that this wasn't meant to be included? |
| web/chat/chat-message-list.react.js | ||
| 377 ↗ | (On Diff #14131) | Why do we need to use optional chaining here? Aren't we guaranteed to have a valid threadInfo and aren't valid threadInfos guaranteed to have an id property? |
| 412 ↗ | (On Diff #14131) | Can this be threadInfo.id? |
| web/chat/chat-message-list-container.css | ||
|---|---|---|
| 10 ↗ | (On Diff #14131) | Totally agree. As the diff focus on moving code, I didn't want to introduce unnecessary changes. |
| web/chat/chat-message-list-container.react.js | ||
| 23–32 ↗ | (On Diff #14131) | I think we can use activeChatThreadID directly. As in my comment above, I didn't want to introduce unnecessary changes. I think it's the best to cover these improvements in separate diff. |
| 82 ↗ | (On Diff #14131) | Yes, it was added accidentally. Thanks for catching. I have no idea how could I miss it. |
| web/chat/chat-message-list.react.js | ||
| 377 ↗ | (On Diff #14131) | Good catch, we don't need it. Thanks |
| 412 ↗ | (On Diff #14131) | Yes, the same above in line 401, I suppose. It's probably because the threadInfo was an optional before my changes in this file. |
Comment Actions
Fix optional chaining issues after Atul's review.
Remove duplicated part of code, that was not removed after moving some parts of the logic to the parent component.
| web/chat/chat-message-list-container.react.js | ||
|---|---|---|
| 44 | Shouldn't we add types here? | |