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

Align vscode: Set theme attributes to webview body element #10493

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

EstherPerelman
Copy link
Contributor

@EstherPerelman EstherPerelman commented Nov 30, 2021

Signed-off-by: Esther Perelman [email protected]

What it does

Align with vscode the ability to set webview style for a specific theme by adding the body data attributes: data-vscode-theme-name & data-vscode-theme-kind (See details here)

image

How to test

  1. In theia project install webview-sample from sample-vsix.zip or download from here
  2. Ctrl+shift+p + Cat coding: Start cat coding session
  3. Open the developer tools and inspect the webview image - search for the body element...
  4. Select the Preferences icon -> Color Theme and navigate between the themes
  5. See the body attributes changing accordingly to the selected theme (you can use vscode theme extensions from the market place)

Review checklist

Reminder for reviewers

@msujew msujew added theming issues related to theming vscode issues related to VSCode compatibility labels Nov 30, 2021
@msujew msujew self-requested a review November 30, 2021 11:55
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I can confirm that the added body attributes are applied correctly and are available during theme selection (using data-vscode-theme-kind="vscode-light" in the example):

2021-11-30.14-30-09.mp4

@vince-fugnitto Do the changes on getActiveTheme (in particular the return type) warrant an entry in the breaking changes log?

@vince-fugnitto
Copy link
Member

@vince-fugnitto Do the changes on getActiveTheme (in particular the return type) warrant an entry in the breaking changes log?

Yes, changing the return value of a method, even if it is protected, would be a breaking change and should be added to the changelog 👍

@EstherPerelman
Copy link
Contributor Author

Hi @msujew @vince-fugnitto thanks for your comments,
I pushed the changelog...

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks, the changes look good to me 👍

  • Webview body-property based styling works for the different theme-types/names
  • The previously set activeTheme class still works correctly

@msujew
Copy link
Member

msujew commented Dec 6, 2021

@EstherPerelman Do you mind resolving the conflict in the changelog? Afterwards, I will merge this PR.

@EstherPerelman
Copy link
Contributor Author

@EstherPerelman Do you mind resolving the conflict in the changelog? Afterwards, I will merge this PR.

Done! thank you

@EstherPerelman
Copy link
Contributor Author

@msujew why didn't you merge it? there are conflicts again... :(

@msujew
Copy link
Member

msujew commented Dec 14, 2021

@EstherPerelman Sorry, I didn't notice the update on this PR. If you update the conflicting files again, I will merge it today.

@EstherPerelman
Copy link
Contributor Author

@EstherPerelman Sorry, I didn't notice the update on this PR. If you update the conflicting files again, I will merge it today.

rebased... thanks!

@msujew msujew merged commit 8bded61 into eclipse-theia:master Dec 14, 2021
@vince-fugnitto vince-fugnitto added this to the 1.21.0 milestone Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theming issues related to theming vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants