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.

Sign up for GitHub

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

Jump to bottom

#248 Support VSCode Codicons #249

Merged
merged 1 commit into from
Oct 18, 2021
Merged

#248 Support VSCode Codicons #249

merged 1 commit into from
Oct 18, 2021

Conversation

ndoschek
Copy link
Contributor

@ndoschek ndoschek commented Oct 14, 2021 •
edited
Loading

Fixes #248

  • Support Codicons via @vscode/codicons
    • Migrate command-palette to codicons
    • Fix loading indicator in command-palette
  • Provide util methods to create codicon CSS classes
  • Keep FontAwesome icons and show that both icon sets can be used in example
    • Class diagram example: command palette uses codicons, class popup uses font-awesome icon (info-circle)

@gitpod-io
Copy link

gitpod-io bot commented Oct 14, 2021

This was referenced Oct 14, 2021
Closed
Closed
planger
planger requested changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thanks @ndoschek! Looks good!
However, the codicon icons don't seem to show for me. I get just squares for the command palette eye and for the loading indicator.

image

The info icon from fontawesome works fine.

src/features/command-palette/command-palette.ts Show resolved Hide resolved
@ndoschek
Copy link
Contributor Author

ndoschek commented Oct 18, 2021 •
edited
Loading

Thanks @planger !
Did you build all packages before testing? The codicons need a new dependency, and it seems that this is missing in your case. I tried it with a clean repo and rebuilt all packages and it works for me. Could you maybe give it another try, thanks!

@planger
Copy link
Contributor

planger commented Oct 18, 2021

Thanks for the improvement!
Unfortunately, I still get squares. I did rebuild. Now I even tried with a git clean and a different browser, but to no avail.
Can you please try with a git clean -xfd?
If that works still works on your machine, I'll debug into it to see what's wrong on my end.
Thanks!

@ndoschek
Copy link
Contributor Author

Oh that's too bad! Yes I executed a git clean -xdf beforehand already, and it still works on my end.
The crucial part seems to be the linked codicon.css file from the node_modules in class-diagram.html (line 10) . Could you have a look if this css can be correctly resolved in your case?

image

@tortmayr
Copy link
Contributor

tortmayr commented Oct 18, 2021 •
edited
Loading

@ndoschek FYI: I can reproduce the behavior that @planger mentioned when opening the PR in gitpod
Screenshot from 2021-10-18 11-16-01

@ndoschek
Copy link
Contributor Author

Thanks @tortmayr, then I will investigate the problem there!

@ndoschek
#248 Support VSCode Codicons
b5cf6b5 
- Support Cocidons via @vscode/codicons
  - Migrate command-palette to codicons
  - Fix loading indicator in command-palette
- Provide util methods to create codicon CSS classes
- Keep FontAwesome icons and show that both icon sets can be used in example
  - Class diagram example: command palette uses codicons, class popup uses font-awesome icon (info-circle)
@ndoschek
Copy link
Contributor Author

I updated the webpack config such that the codicon font file can be accessed properly, so I could remove the relatively linked stylesheet from class-diagram.html.

planger
planger approved these changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thanks, now it works! Code looks good.

@planger planger merged commit 5ae1416 into eclipse-sprotty:master Oct 18, 2021
@ndoschek ndoschek deleted the ndoschek/issues/248 branch October 18, 2021 11:15
@spoenemann
Copy link
Contributor

The new dependency to @vscode/codicons was added to devDependencies, but shouldn't it be rather in dependencies? The library code now depends on it, right?

@ndoschek
Copy link
Contributor Author

Ah you are right, thanks for catching that! I opened a PR for that: #250

@tortmayr tortmayr mentioned this pull request Sep 12, 2022
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@planger planger planger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support VSCode Codicons
4 participants
@ndoschek @planger @tortmayr @spoenemann