PROTOTYPE: Lightweight rendering of fixed scroll height #273207
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements lightweight rendering for fixed scroll height in the chat component, focusing on visual improvements and icon transitions for the thinking state UI. The changes primarily affect the header button styling and icon management system.
Key changes:
- Simplified icon management by removing separate status icon element and integrating icon transitions directly into the header button
- Updated CSS styling for the header button and fixed height controller with new colors, spacing, and border treatments
- Streamlined code by removing unused DOM utilities import
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/chat/browser/media/chat.css |
Updated styling for chat thinking fixed header and height controller with new colors, spacing, and border treatments |
src/vs/workbench/contrib/chat/browser/chatContentParts/chatThinkingContentPart.ts |
Removed separate status icon element and integrated icon transitions directly into header button, cleaned up icon handling logic |
| const spinnerEl = dom.h(ThemeIcon.asCSSSelector(ThemeIcon.modify(Codicon.loading, 'spin'))); | ||
| this.statusIcon.appendChild(spinnerEl.root); | ||
| button.element.appendChild(this.statusIcon); | ||
| // Start with spinner icon, will transition to chevron when finished |
There was a problem hiding this comment.
[nitpick] The comment mentions 'transition to chevron when finished' but doesn't explain the actual mechanism. Consider expanding the comment to reference that the transition happens in the layout() method when the 'finished' class is added to the container.
| // Start with spinner icon, will transition to chevron when finished | |
| // Start with spinner icon. The icon will transition to a chevron when finished, | |
| // which happens in the layout() method when the 'finished' class is added to the container. |
| this.fixedCollapsed = collapsed; | ||
| this.fixedContainer.classList.toggle('collapsed', collapsed); | ||
| if (this.headerButton) { | ||
| // Update the button icon chevron direction when finished |
There was a problem hiding this comment.
The comment states 'when finished' but the condition checks if the container already has the 'finished' class. This comment is misleading as it suggests the icon is being updated upon completion, when it's actually only updating the chevron direction during collapse/expand operations after completion. Consider revising to: 'Only update chevron direction if thinking is finished'.
| // Update the button icon chevron direction when finished | |
| // Only update chevron direction if thinking is finished |
sbatten
changed the title
|
|
||
| import { $, clearNode } from '../../../../../base/browser/dom.js'; | ||
| import * as dom from '../../../../../base/browser/dom.js'; | ||
| import { IChatThinkingPart } from '../../common/chatService.js'; |
There was a problem hiding this comment.
[nitpick] The unused import * as dom was removed, but the remaining individual import of $ and clearNode from the same module suggests considering consolidating the import statement to use the namespace pattern consistently throughout the file for better maintainability.
| // Update the button icon chevron direction when finished | ||
| if (this.fixedContainer.classList.contains('finished') && this.headerButton) { |
There was a problem hiding this comment.
The check for this.fixedContainer.classList.contains('finished') creates a dependency on the 'finished' class being set before the icon can be updated. This could lead to the icon not updating if setFixedCollapsedState is called before the 'finished' class is added. Consider handling the icon update logic more explicitly based on the component's state rather than class presence.
|
@sbatten yes, thats some nice progress for the issue. I wonder if the expanded body of the thinking part needs to be so complex in rendering: whats the idea behind the bullets and the line going from top to bottom on the left? It looks so different from anything else in Chat that I wonder if it isn't too much. What if we would simply render the thinking text after you expand without much borders and bullets? |
Other collapsible chat widgets use the rounded corner border as well, like search results. I think this helps with the visual hierarchy personally. As for the bullets. This is supposed to convey "chain of thought" It also helps with the visual hierarchy and helps distinguish the thinking section from the rest, but I'll let @justschen comment. |
This prototype updates the fixed height scrolling thinking part to be consistent with the general tool invocation (e.g. Search tool invocation)
cc @bpasero per #273029