-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Fix welcome view links missing hover color #121835
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
Fix welcome view links missing hover color #121835
Conversation
| } | ||
| })); | ||
|
|
||
| const onMouseOver = domEvent(this.el, 'mouseover'); |
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'd recommend to instead follow the existing pattern for styling links on hover using CSS, for example here's how the search view does it:
vscode/src/vs/workbench/contrib/search/browser/searchView.ts
Lines 1977 to 1981 in f547ada
| const activeLink = theme.getColor(textLinkActiveForeground); | |
| if (activeLink) { | |
| collector.addRule(`.monaco-workbench .search-view .message a:hover, | |
| .monaco-workbench .search-view .message a:active { color: ${activeLink}; }`); | |
| } |
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 implemented it but realized it doesn't really work because Link is used in a few different places:
src/vs/workbench/browser/parts/viewssrc/vs/workbench/contrib/welcome/gettingStarted/browsersrc/vs/workbench/contrib/workspace/browser
So it doesn't look like there's a safe CSS selector that can cover all those cases and potential future consumers of Link.
What's your thought?
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.
You might add a class to the link's a element and style based on that, but I'll leave further review to @joaomoreno as he owns this code.
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.
Oh yes, that sounds like the way to do it. I'll have a go while waiting for @joaomoreno's review
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.
No luck with registerThemingParticipant yet, I'll wait for @joaomoreno 's review for now
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.
Any thoughts @joaomoreno ?
glaukiol1
left a 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.
solid fix, thanks for contributing, also, please make a default color! thanks
|
Thanks! 🍻 Ended up cleaning up quite a bit how we style links. |
This PR fixes #121521