-
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
vscode: support icons contribution point #12912
Conversation
…into icon-contributions
This commit supports the icon contributions point. It is working well, but there is some code simplification and cleanup still to do. Signed-off-by: Vlad Arama <[email protected]>
This commit cleans up the remaining console.log statements and adds disposable methods to the `PluginIconService` class. Signed-off-by: Vlad Arama <[email protected]>
This commit simplifies the `PluginIconService` class by adding helper methods and decomposing the large code into smaller manageable functions. Signed-off-by: Vlad Arama <[email protected]>
This commit fixes a few linting errors related to file-headers. Signed-off-by: Vlad Arama <[email protected]>
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.
Hi Vlad, thanks for the PR. There seems to be a bit of unused code and the layering (core vs. monaco) needs fixing. Functionally, it seems to do the job.
@vladarama Can you tell when you want to look at Thomas comments? I think this PR would be great to have for the upcoming release... |
Hey @JonasHelming, |
@vladarama Sounds great! Please keep in mind that we have a release on Thursday this week, maybe this one can make it in :-) |
@vladarama could you address the remaining open comment, please? |
@injectable() | ||
export class MonacoIconRegistry implements IconRegistry { | ||
|
||
readonly onDidChange: Event<void>; |
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.
Is the "onDidChange" event connected to anything? If not, why do we have it?
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.
Thanks for the reviews Thomas,
The event doesn't seem to be connected to anything. Its been removed 👍
@vladarama one more question I have, but then I think we're good to go. |
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 now.
What it does
Fixes: #12355
This PR adds support for the
contributes.icon
contribution point from VSCode. This fixes a number of issues where extension icons were invisible such as many of the Gitlens icons.How to test
You can test the functionality of this PR by downloading an extension which contributes its icons through the icons contribution point. One such example is Gitlens.
Extensions
viewReview checklist
Reminder for reviewers