Skip to content
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

Add @vscode/codicons dependency to core package #9828

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

msujew
Copy link
Member

@msujew msujew commented Aug 3, 2021

What it does

Closes #9826

Adds the @vscode/codicons package to the core Theia package. Currently codicons are only available once monaco has been loaded to the DOM. This loads any codicons at startup without the need to include monaco in Theia. I've confirmed that all currently used codicons classes are contained in the referenced css file, except for tree-item-expanded, which I've replaced with chevron-down.

cc @vince-fugnitto does this require a CQ?

How to test

  1. Confirm that everything works as expected with this change in place.
  2. Modify the example packages to remove any references to monaco packages. See the following dependencies section:
"dependencies": {
    "@theia/bulk-edit": "1.16.0",
    "@theia/callhierarchy": "1.16.0",
    "@theia/core": "1.16.0",
    "@theia/file-search": "1.16.0",
    "@theia/filesystem": "1.16.0",
    "@theia/git": "1.16.0",
    "@theia/keymaps": "1.16.0",
    "@theia/messages": "1.16.0",
    "@theia/metrics": "1.16.0",
    "@theia/mini-browser": "1.16.0",
    "@theia/navigator": "1.16.0",
    "@theia/preferences": "1.16.0",
    "@theia/scm": "1.16.0",
    "@theia/scm-extra": "1.16.0",
    "@theia/workspace": "1.16.0"
},
  1. Compile everything and start the application. Confirm that any codicons are correctly rendered. (e.g. chevrons for tree explorers, settings gear in the bottom left corner)

Review checklist

Reminder for reviewers

@vince-fugnitto
Copy link
Member

cc @vince-fugnitto does this require a CQ?

I don't think so (while we always can to be safe), as in our guide for new dependencies we can do the check ourselves, and I confirmed that the license is "creative commons" which is an approved license.

Copy link
Contributor

@linhanyu linhanyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can approve those change were effective in our electron( Macos / Windows )

@vince-fugnitto vince-fugnitto added monaco issues related to monaco ui/ux issues related to user interface / user experience labels Aug 3, 2021
@msujew msujew force-pushed the msujew/codicons-core branch from a6abf2f to cbf9929 Compare August 3, 2021 12:08
@vince-fugnitto
Copy link
Member

vince-fugnitto commented Aug 3, 2021

It looks like we finally ran into the issue where codicons are not available, something we predicted a while ago if monaco was not included :)

With this change, we should be able to update all icons in the framework to codicons without any worries 👍

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that the change fixes the reported issue 👍

@vince-fugnitto vince-fugnitto merged commit 7e09e93 into eclipse-theia:master Aug 6, 2021
@msujew msujew deleted the msujew/codicons-core branch March 8, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

electron: Setting Icon disappear if not editor opened.
3 participants