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

Webview codicons: styling and interactions with codicons #352

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jan-dolejsi
Copy link
Contributor

Added some more representative example of codicons usage:

  • styling codicons
  • interacting with codicons

I also took the liberty to remove the references to uninvolved animals and replaced the demo.gif.

@jan-dolejsi jan-dolejsi changed the title styling and interactions with codicons Webview codicons: styling and interactions with codicons Oct 18, 2020
@jan-dolejsi
Copy link
Contributor Author

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?

@miguelsolorio
Copy link
Contributor

miguelsolorio commented Nov 24, 2020

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.

@jan-dolejsi
Copy link
Contributor Author

jan-dolejsi commented Nov 28, 2020

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.
My motivation was two fold:

  1. the sample had nothing to do with cats, so needed some cleaning up
  2. the sample missed an important value of icon fonts in vscode webviews: the font is automatically styled into the light/dark theme, so the extension developer does not need to band over backwards to achieve the same effect with svg icons.
  3. Plus they are great looking and may be used in multiple different UX scenarios, not just visualized as part of text (which is the only thing the orig sample was showing).

@miguelsolorio
Copy link
Contributor

the sample had nothing to do with cats, so needed some cleaning up

this is fair, I believe this was inherited by the previous webview examples

the sample missed an important value of icon fonts in vscode webviews: the font is automatically styled into the light/dark theme, so the extension developer does not need to band over backwards to achieve the same effect with svg icons.

the current sample does use color tokens for icons so they are automatically styled

https://github.com/microsoft/vscode-extension-samples/blob/master/webview-codicons-sample/media/styles.css#L32

Plus they are great looking and may be used in multiple different UX scenarios, not just visualized as part of text (which is the only thing the orig sample was showing).

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

webview-codicons-sample/media/buttons.js Outdated Show resolved Hide resolved
webview-codicons-sample/src/extension.ts Outdated Show resolved Hide resolved
uniquemo
uniquemo previously approved these changes Dec 3, 2020
@jan-dolejsi jan-dolejsi requested a review from mjbvz December 6, 2020 23:23
Base automatically changed from master to main February 22, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants