Closed Bug 965814 Opened 6 years ago Closed 6 years ago

Make the shader editor look good on the light theme

Categories

(DevTools Graveyard :: WebGL Shader Editor, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 2 obsolete files)

Followup for bug 957117.
Depends on: 957117
Attached patch se-theme.patch (obsolete) — Splinter Review
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8367970 - Flags: review?(bgrinstead)
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)
Attachment #8367970 - Flags: review?(bgrinstead)
(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.
Attached patch v2 (obsolete) — Splinter Review
Addressed comments.
Attachment #8367970 - Attachment is obsolete: true
Attachment #8368030 - Flags: review?(bgrinstead)
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).
Attachment #8368030 - Flags: review?(bgrinstead) → review+
Attached patch v3Splinter Review
Yup, didn't need it.
Attachment #8368030 - Attachment is obsolete: true
Attachment #8368078 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/83c9104a63ba
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.