-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generic hover service support #11869
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that the tooltips work really well with the new hover service 👍
I might actually open an issue in vscode for them to standardize using these tooltips everywhere for a more consistent look and feel (ex: explorer file paths, tree-view hover, toolbar item hover).
One improvement would be to adjust the hr
rule for the vsx-extensions
view as the horizontal rule is not styled similarly to vscode since we have a padding on each side:
hr.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, so I endorse @vince-fugnitto approval. Only one minor comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me 👍
I confirmed that my previous comment about the styling was addressed.
protected getHoverDelay(): number { | ||
return Date.now() - this.lastHidHover < 200 | ||
? 0 | ||
: this.preferences.get('workbench.hover.delay', isOSX ? 1500 : 500); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msujew, I notice that in Theia with this change, if any hover has recently been rendered, then any other hover, of the same or different kind, will be rendered immediately, but in VSCode, it appears to differ by source. So e.g. if you hover over an extension tree node, get the hover, then hover over a sidebar tabbar icon, the tabbar icon's title hover will appear immediately, but in VSCode, it will only hover after a delay.
@@ -135,6 +135,7 @@ import { bindStatusBar } from './status-bar'; | |||
import { MarkdownRenderer, MarkdownRendererFactory, MarkdownRendererImpl } from './markdown-rendering/markdown-renderer'; | |||
import { StylingParticipant, StylingService } from './styling-service'; | |||
import { bindCommonStylingParticipants } from './common-styling-participants'; | |||
import { HoverService } from './hover-service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_
What it does
Fixes #11594
Fixes #10867
Also addresses a comment I've left in another PR #11083 (comment).
Uses the
StatusBarHoverManager
(nowHoverService
) as a base for building a generic hover service for markdown/string/html display as hovers/tooltips. This retires theTooltipService
(introduced in #10108), which was the source of the error messages in #11594.One feature that is missing here is that the hover could appear at the cursor position, right now it's bound to the position of the target element. I plan to add that in a separate PR later on.
How to test
resolveTreeItem
toTreeDataProvider
#11708)Review checklist
Reminder for reviewers