Update some Icons #6993
Update some Icons #6993
Conversation
|
@JesterOrNot please squash in one commit, there is no many changes to keep them separate @vince-fugnitto i am not sure whether we need a CQ for it, we should check it |
|
it should not close the original issue, the proper fix should consume icons from monaco or from vscode icons and provide easy way to catch up. But let's accept a PR as a temporary measure. |
|
Rebased! |
|
@JesterOrNot thank you by not commit message is not informative, you could do commit amend to edit last commit message |
There was a problem hiding this comment.
I don't think we should only update only subset of icons while not updating others as it will make the application look inconsistent. There are a few icons omitted by the pull-request which are noticeable now compared to those that are updated.
I think we should perhaps register a cq for the entire vscode-icons repo so we can consume them without worry? @marcdumais-work
+1 . That's what we did previously I think: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=20694 Do we know which version of the Assuming the updated icons were not in that version, we'll need a new CQ and need to mention which SHA version it corresponds-to. |
I don't think we ever consumed from the |
I do not see any clue that this repo is meant to be packaged or how it's consumed outside Theia. I wonder if we could do something similar to what Anton did for the built-ins: create a repo that wraps-around With something like that we would no longer need "Type B" CQs when we update the icons, I think, since we would no longer copying them to our repo. The new dependency we consume would only be checked, using the "self-check", to confirm license compliance, whenever we step the dependency version. |
The CQ above links to that repo. You're right that before we used to pull icons directly from the main vscode repo. update: not sure we actually did anything with that CQ, but it was registered and approved. |
|
Amended! |
|
@JesterOrNot please be sure to sign-off you commit and also sign the Eclipse Contributor Agreement (ECA), they are the reason for the failed check. Also, can you update your commit message as Anton previously mentioned? |
|
How do I signoff commits? |
|
I keep re validating but it still consistently fails |
You can: (taken from sign-off) |
|
Is this any better? |
The For the rest, the following #6993 (review) is still a concern along with the licensing aspect mentioned of registering their entire repo during a cq. |
vince-fugnitto
added
the
CQ Required (deprecated)
|
Is there anything I can do to help resolve this? |
Where did you copy the icons from (repo and exact version)? We can take care of the CQ once we know. |
|
@JesterOrNot it looks like a previous CQ has been already registered for the repo. If we stick to using icons at this given commit microsoft/vscode-icons@9c90ce8 we should no longer need a new CQ :) Do you mind updating our icons based on this commit? If we need newer icons (or fixes) then we will need a new CQ (please let us know if it is the case). |
|
Sure! |
|
Wait! VSCode updated their items again. The ones in that commit are outdated, so if we use them this PR is null as we would still be outdated. What should we do? |
|
I am so sorry. I try my best updating them |
|
does inverse = light? |
JesterOrNot
force-pushed
the
icons
branch
5 times, most recently
from
4893721
to
66d8eb6
Compare
I think so, depending if you mean the icon itself or the theme it's meant to be used-with. From what I can see, the icons are named like so: Icons used in
Icon used in
|
|
What Icon controls that |
Investigating a bit it looks to come from Font Awesone: "fa fa-plus" |
|
This one is as well from Font Awesome: "fa fa-undo" |
|
Ok, because it seems out of date with the modern VSCode Icon we should mention the font awesome linked icons in the main issue. Also in terms of automation, we could make a theia fork and npmize it and pull request to the main one, we'd probably need another CQ, however, if they don't except the PR we could maintain the fork. |
ok - please do.
Are you suggesting proposing updated icons to the Font Awesome project? I assume the corresponding VS Code icons are part of the Here's what these look-like in VS Code: |
|
For the second part I meant as the real solution for #6115 |
JesterOrNot
force-pushed
the
icons
branch
2 times, most recently
from
3876c43
to
0e8d74d
Compare
Signed-off-by: Sean Hellum <seanhellum45@gmail.com> This commit updates the icons to match the latest icon sets from VS Code
|
@JesterOrNot the For example, it is possible to update the explorer's icon by setting it's icon class |
|
So do we close this PR? |
It's up to you, we can either restart by amending the following pull-request or start clean and open a new one. |
|
What do you think would we need to restart either way? |
I think so, there will no longer be a need to copy over all the images we require and instead reference the new icons while updating the styling a bit. It might be a lot cleaner to restart. |
|
OK! |
What it does
#6115
I updated
someicons manuallyIcons list obtained like so:
Icons done:
Icons TODO:
Icons Not Applicable (not from VS Code):
How to test
Check the new icons out!
Review checklist
Reminder for reviewers
Sean Hellum seanhellum45@gmail.com