-
Notifications
You must be signed in to change notification settings - Fork 3.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
Webview codicons: styling and interactions with codicons #352
base: main
Are you sure you want to change the base?
Conversation
Gate keepers, I have another sample I would like to push as a PR (in the same fork branch), so thought this one would be fairly quick to get in. Is there something I need to do in order to get this reviewed and merged? |
Hey @jan-dolejsi, thanks for creating the PR as I'm reviewing it now. While I appreciate the additional references for styled icons and mouse events, i'm not sure whether those should be added since we try to keep the samples as basic as possible. @mjbvz thoughts? We also have other webview samples that have the same structure as this one so we'd need to consider those as well. |
Simplicity and conciseness is indeed most important for samples. I will be happy to enlighten it again. I would welcome feedback about what is most useful.
|
this is fair, I believe this was inherited by the previous webview examples
the current sample does use color tokens for icons so they are automatically styled
this is probably where I have hard time adding these additional scenarios. it makes assumptions and adds additional files the a user may not want. this is where I prefer it to be "simplified" and only show how to add the icons. There are so many different ways that you can use the icon that we can't possibly cover, which is why a lot of our samples have basic examples. Will leave some more feedback in the PR. |
|
||
div.styledIcon .codicon { | ||
font-size: 50px; | ||
color: green; |
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.
this is referencing a color that isn't a color token, something we avoid doing. Something like var(--vscode-debugIcon-startForeground
would be preffered.
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.
That's a really good point. Fixed.
Added some more representative example of codicons usage:
I also took the liberty to remove the references to uninvolved animals and replaced the
demo.gif
.