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

Fix webview theme styles #6155

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Fix webview theme styles #6155

merged 1 commit into from
Sep 11, 2019

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented Sep 10, 2019

What it does

Add material colors into webviews.

What issues does this PR fix or reference?

#4179

How to test

Try to use this 'asciidoctor-vscode' extension
https://marketplace.visualstudio.com/items?itemName=joaompinto.asciidoctor-vscode.
Check all themes.

Screenshot from 2019-09-10 21-06-16
Screenshot from 2019-09-10 21-05-52

@olexii4 olexii4 requested a review from a team as a code owner September 10, 2019 18:07
@akosyakov akosyakov added plug-in system issues related to the plug-in system webviews issues related to webviews labels Sep 11, 2019
@azatsarynnyy
Copy link
Member

I've tested it with K8s extension.

Before:
image

After:
image

Looks much better now 🙂
Thanks! 👍

@akosyakov
Copy link
Member

akosyakov commented Sep 11, 2019

@azatsarynnyy that's cool!

I just cannot grasp what PR does. It uses and unuses exactly the same styles in core and by doing it somehow fix the issue in the plugin system? It sounds as a very implicit dependency and also unnecessary for switching color themes.

@akosyakov akosyakov added the shell issues related to the core shell label Sep 11, 2019
@AndrienkoAleksandr
Copy link
Contributor

@akosyakov In the plugin system we display plugin webview in the iframe. And we need provide styles to this iframe.

@vinokurig
Copy link
Contributor

Tested against GitHub plugin:
Before:
screenshot-che-che 192 168 39 115 nip io-2019 09 11-12-48-32
After:
screenshot-localhost-3000-2019 09 11-12-48-16
Vscode:
Screenshot from 2019-09-11 12-47-52
Looks much better 👍 It is far from the vscode view but at least it is readable.

@olexii4
Copy link
Contributor Author

olexii4 commented Sep 11, 2019

I just provide Theia styles and variables into webview iframes(add materialcolors).
Now it looks like regular styles for Theia 'p-Widget' class.
In this place, we can restyle webview iframes depends on variables:
https://github.com/theia-ide/theia/pull/6155/files#diff-2cf6add9fc7566d40d46a3533b4d5cf7R26
And we can customize it.

@akosyakov
Copy link
Member

In this place, we can restyle webview iframes depends on variables:
https://github.com/theia-ide/theia/pull/6155/files#diff-2cf6add9fc7566d40d46a3533b4d5cf7R26

@olexii4 probably it should be a comment in the code

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code wise it looks now

@vinokurig @azatsarynnyy it would be good if you can double check with recent changes

@olexii4 commits have to be cleaned up and signed off: https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#checklist-sign-off

Signed-off-by: Oleksii Orel <oorel@redhat.com>
@olexii4
Copy link
Contributor Author

olexii4 commented Sep 11, 2019

@akosyakov I have done.

Copy link
Contributor

@vinokurig vinokurig left a comment

Choose a reason for hiding this comment

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

Tested with Github plugin

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

checked with the latest changes - works as expected

@olexii4 olexii4 merged commit 61ee5aa into master Sep 11, 2019
@olexii4 olexii4 deleted the CHE-14196 branch September 11, 2019 12:04
@kittaakos
Copy link
Contributor

FYI: the build is broken on the master: https://travis-ci.org/theia-ide/theia/builds/583626693

lerna ERR! /home/travis/build/theia-ide/theia/packages/core/lib/browser/theming.js:43
1586lerna ERR! require('../../src/browser/style/materialcolors.css').use();
1587lerna ERR!                                                       ^
1588lerna ERR! 
1589lerna ERR! TypeError: require(...).use is not a function
1590lerna ERR!     at Object.<anonymous> (/home/travis/build/theia-ide/theia/packages/core/lib/browser/theming.js:43:55)
1591lerna ERR!     at Module._compile (internal/modules/cjs/loader.js:778:30)
1592lerna ERR!     at Module.replacementCompile (/home/travis/build/theia-ide/theia/node_modules/nyc/node_modules/append-transform/index.js:58:13)

Is it related?

@kittaakos
Copy link
Contributor

Also:

Screen Shot 2019-09-11 at 14 50 42

Screen Shot 2019-09-11 at 14 51 04

@AlexTugarev
Copy link
Contributor

@olexii4 could you please take care of the broken master build?

@olexii4
Copy link
Contributor Author

olexii4 commented Sep 11, 2019

@AlexTugarev Ok. I will fix PR

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Sep 11, 2019

Also:

Screen Shot 2019-09-11 at 14 50 42

Screen Shot 2019-09-11 at 14 51 04

Can we enforce that PRs can not be merged if the CI (Travis and AppVeyor) are failing?

  • We can enforce at least that Travis must be passing before enabling the merge, and perhaps AppVeyor as well (since it is much more stable than it used to be)

Opened the issue #6167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system shell issues related to the core shell webviews issues related to webviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants