965814 - Make the shader editor look good on the light theme

Status: RESOLVED FIXED

(DevTools Graveyard :: WebGL Shader Editor, defect)

Description

[Victor Porof [:vporof][:vp]]

Followup for bug 957117.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: se-theme.patch

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8367970 [details] [diff] [review]
se-theme.patch

Review of attachment 8367970 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good on both themes.  Just a couple of notes.

::: browser/devtools/shadereditor/shadereditor.xul
@@ -18,5 @@
>            src="chrome://browser/content/devtools/theme-switching.js"/>
>  
>    <script type="application/javascript" src="shadereditor.js"/>
>  
> -  <vbox id="body" flex="1">

I believe you can remove the body ID here, since it isn't used anymore

::: browser/themes/shared/devtools/shadereditor.inc.css
@@ +97,5 @@
> +  color: #f5f7fa; /* Light foreground text */
> +}
> +
> +.theme-light .editor-label {
> +  background: url(background-noise-toolbar.png), #f0f1f2; /* Light toolbars */

background-noise-toolbar.png is not actually used in the light theme for the side-menu-widget-container, so I would suspect that we shouldn't be using it on the editor-label or notice-container here (as we want this to match the list on the left)

Comment 3

[Victor Porof [:vporof][:vp]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8367970 [details] [diff] [review]
> se-theme.patch
> 
> Review of attachment 8367970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good on both themes.  Just a couple of notes.
> 
> ::: browser/devtools/shadereditor/shadereditor.xul
> @@ -18,5 @@
> >            src="chrome://browser/content/devtools/theme-switching.js"/>
> >  
> >    <script type="application/javascript" src="shadereditor.js"/>
> >  
> > -  <vbox id="body" flex="1">
> 
> I believe you can remove the body ID here, since it isn't used anymore
> 
> ::: browser/themes/shared/devtools/shadereditor.inc.css
> @@ +97,5 @@
> > +  color: #f5f7fa; /* Light foreground text */
> > +}
> > +
> > +.theme-light .editor-label {
> > +  background: url(background-noise-toolbar.png), #f0f1f2; /* Light toolbars */
> 
> background-noise-toolbar.png is not actually used in the light theme for the
> side-menu-widget-container, so I would suspect that we shouldn't be using it
> on the editor-label or notice-container here (as we want this to match the
> list on the left)

This notice is shown only when the tool is completely empty. I could remove it from the editor label though.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2

Addressed comments.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8368030 [details] [diff] [review]
v2

Review of attachment 8368030 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with one minor update (unless if there is a reason theme-body is duplicated)

::: browser/devtools/shadereditor/shadereditor.xul
@@ +44,5 @@
>               value="&shaderEditorUI.emptyNotice;"/>
>      </hbox>
>  
> +    <box id="content"
> +         class="devtools-responsive-container theme-body"

I don't believe that you need theme-body to be set here (it is already on the container vbox).

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v3

Yup, didn't need it.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/83c9104a63ba

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83c9104a63ba