fix: fixes icons not showing when hovering quick pick checkboxes#285250
fix: fixes icons not showing when hovering quick pick checkboxes#285250TylerLeonhardt merged 4 commits intomicrosoft:mainfrom
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where theme icons were not rendered correctly when hovering over quick pick checkboxes. The fix involves changing the content property in the hover configuration from a plain string to an IMarkdownString object with supportThemeIcons: true.
Key changes:
- Updated the
setupDelayedHovercall in theToggleconstructor to use an object-based content format that enables theme icon support
| this.domNode = document.createElement('div'); | ||
| this._register(getBaseLayerHoverDelegate().setupDelayedHover(this.domNode, () => ({ | ||
| content: this._title, | ||
| content: { value: this._title, supportThemeIcons: true }, |
There was a problem hiding this comment.
can you instead have the title options take in string | IMarkdownString | HTMLElement? since it's just used in the content property of the hover?
There was a problem hiding this comment.
ah, this way the ones implementing the class are responsible for how the content is shown, much better.
There was a problem hiding this comment.
@Tyriar I am not sure where the quick pick in your screenshot comes from, can't find it in vscode or vscode-copilot-chat. If that option is coming from one of those two repos let me know, if it's an option from an extension I guess they'll fix it.
There was a problem hiding this comment.
Yeah I couldn't find that either. I am curious what @Tyriar thinks of this approach. How much massaging should we do in this hover? Should we strip/render the icons in the string case?
I was also thinking about the aria label as well... we for sure want to not have the icons in that... but right now it's inferred by the label.
There was a problem hiding this comment.
Opted for stripping the icons.
TylerLeonhardt
left a comment
TylerLeonhardt
left a comment
There was a problem hiding this comment.
Awesome. Thanks for fixing this!
3 participants
Fixes issue #284323
A simple fix that adds
supportThemeIcons: trueto the content of the setupDelayedHover function so that icons are correctly shown.