fix(chat-messages): improve chat container layout and examples#1810
fix(chat-messages): improve chat container layout and examples#1810Killusions wants to merge 1 commit intomainfrom
Conversation
- add max-inline-size constraint to message bubble and wrapper to prevent overflow - fix attachment preview backdrop click behavior - use index-based tracking for message list and add scroll-to-bottom on message send - update example action labels for clarity
There was a problem hiding this comment.
Code Review
This pull request renames chat actions from 'Copy' to 'Export' and 'Good response' to 'Add to list,' updates message bubble styling with max-inline-size, and adds automatic scrolling logic. Feedback suggests merging redundant CSS selectors and reverting the @for loop tracking to use the message object instead of $index to prevent re-rendering issues. Furthermore, log messages should be updated for terminology consistency, and a redundant setTimeout for scrolling should be removed.
| .message-wrapper { | ||
| min-inline-size: 0; | ||
| max-inline-size: 100%; | ||
| } |
There was a problem hiding this comment.
| <si-chat-container colorVariant="base-0"> | ||
| @for (message of messages(); track message) { | ||
| <si-chat-container #chatContainer colorVariant="base-0"> | ||
| @for (message of messages(); track $index) { |
There was a problem hiding this comment.
Using $index for tracking in @for is generally discouraged when items can be removed from the list (as seen with the 'Delete message' action on line 187 of the component). Tracking by the message object itself is more efficient in this case, as it prevents unnecessary re-renders of subsequent items when a message is deleted from the middle of the conversation.
| @for (message of messages(); track $index) { | |
| @for (message of messages(); track message) { |
| label: 'Export message', | ||
| icon: 'element-export', | ||
| action: (message: ChatMessage) => | ||
| this.logEvent(`Copy user message ${message.content.slice(0, 20)}...`) |
There was a problem hiding this comment.
The action label was updated to 'Export message', but the log message still refers to 'Copy'. It is recommended to update the log message to maintain consistency with the UI labels, following the Siemens UX writing guidelines for consistent terminology.
| this.logEvent(`Copy user message ${message.content.slice(0, 20)}...`) | |
| this.logEvent('Export user message ' + message.content.slice(0, 20) + '...') |
References
- Use the same words and grammatical forms, lengths, and styles repeatedly to ensure consistency across the UI. (link)
| label: 'Export message', | ||
| icon: 'element-export', | ||
| action: (_message: ChatMessage) => | ||
| this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`) |
There was a problem hiding this comment.
The action label was updated to 'Export message', but the log message still refers to 'Copy'.
| this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`) | |
| this.logEvent('Export user message ' + _message.content.slice(0, 20) + '...') |
References
- Use the same words and grammatical forms, lengths, and styles repeatedly to ensure consistency across the UI. (link)
| label: 'Export message', | ||
| icon: 'element-export', | ||
| action: (_message: ChatMessage) => | ||
| this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`) |
There was a problem hiding this comment.
The action label was updated to 'Export message', but the log message still refers to 'Copy'.
| this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`) | |
| this.logEvent('Export user message ' + _message.content.slice(0, 20) + '...') |
References
- Use the same words and grammatical forms, lengths, and styles repeatedly to ensure consistency across the UI. (link)
| setTimeout(() => { | ||
| this.chatContainer()?.scrollToBottom(); | ||
| }, 0); |
There was a problem hiding this comment.
The setTimeout is redundant here. SiChatContainerComponent already implements a MutationObserver that automatically scrolls to the bottom when content changes, provided that isUserAtBottom is true. Calling this.chatContainer()?.scrollToBottom() synchronously is sufficient to set the internal state and ensure the container scrolls once the new message is rendered in the DOM.
this.chatContainer()?.scrollToBottom();References
- Avoid using asynchronous schedulers or setTimeout if the state being checked is updated synchronously by the same event.
overflow
add scroll-to-bottom on message send
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: