CSS Cleanup on side panels

RESOLVED FIXED in Firefox 42

Status

()

Firefox
Developer Tools: WebIDE
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ednapiranha, Assigned: bstoroz)

Tracking

40 Branch
Firefox 42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8617548 [details]
Firefox_WebIDE.png

Fix the following:

- padding, margins, text layout
- buttons (larger buttons are not styled consistently)

To enable for testing/debugging locally:

1. Go to about:config and set |devtools.webide.enableLocalRuntime| to true and |devtools.webide.sidebars| to true

2. Reopen WebIDE
(Reporter)

Updated

3 years ago
Blocks: 1173127
(Reporter)

Updated

3 years ago
Depends on: 1173133
(Reporter)

Updated

3 years ago
Assignee: nobody → bstoroz
(Assignee)

Comment 1

3 years ago
Created attachment 8633029 [details] [diff] [review]
sidebarsCSS.patch

CSS has been cleaned up, but can probably still use a little work. I'd like to grab the icons used in the former panels but I'm having a hard time inspecting those panels to see how they were incorporated.

Also maybe I can add some conditional text for the wifi/simulator sections if their arrays are empty? i.e. 'No wifi devices found'.
Attachment #8633029 - Flags: feedback?(jfong)
(Assignee)

Comment 2

3 years ago
Created attachment 8633031 [details]
WebIDE-sidebarCSS-v1.png
(Reporter)

Comment 3

3 years ago
Comment on attachment 8633029 [details] [diff] [review]
sidebarsCSS.patch

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

Small non-code things to do:

The commit message on the patch should be `Bug <number> - Clean up WebIDE sidebars CSS. r=jfong` (or whoever is reviewing).

A couple of things to check/change:

- Make sure that the look and feel for the non sidebar version is still the same as before - we want your changes only to be valid for when this pref is enabled. Go to about:config and toggle devtools.webide.sidebars to check.

- The buttons that you've changed should probably have a bit of definition - maybe a slight border on bottom/right for each one? What do you think?

::: browser/devtools/webide/themes/panel-listing.css
@@ +45,4 @@
>    min-width: 130px;
> +  display: block;
> +  background-color: #fff;
> +  border: none;

Although the same either way maybe use border: 0, but not really opposed to just leaving it.

@@ +46,5 @@
> +  display: block;
> +  background-color: #fff;
> +  border: none;
> +  margin-bottom: 1px;
> +  width: 100%;

If we have a width of 100% but padding of 3% that would go over 100, no?

@@ +58,5 @@
> +.panel-item:not(:disabled):hover {
> +  background-color: #CCF0FD;
> +}
> +
> +#project-panel-projects .panel-item, #project-panel-runtimeapps .panel-item {

Maybe put these all on their own lines
Attachment #8633029 - Flags: feedback?(jfong) → feedback+
(Assignee)

Comment 4

3 years ago
Created attachment 8634478 [details] [diff] [review]
WebIDESidebarCSS.patch

New patch adds icons to sidebar buttons, fixes borders, and styles the recently added configure-buttons for simulators.

The sidebar styling does not conflict with or override any of the pre-existing styles on the panels.
Attachment #8633029 - Attachment is obsolete: true
Attachment #8634478 - Flags: review?(jryans)
(Assignee)

Comment 5

3 years ago
Created attachment 8634480 [details]
WebIDE-sidebarCSS-v2.png
Attachment #8633031 - Attachment is obsolete: true
Comment on attachment 8634478 [details] [diff] [review]
WebIDESidebarCSS.patch

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

It looks great to me, thanks for working on this!  Just a few style nits to clean up.

There is a DevTools CSS style guide[1], but it's a bit outdated as far the "splitting files" stuff, we no longer really do that, and we have never done it for WebIDE.

It is helpful for syntax rules we try to follow, though.

[1]: https://wiki.mozilla.org/DevTools/CSSTips

::: browser/devtools/webide/themes/panel-listing.css
@@ +9,3 @@
>  }
>  
>  .panel-item, label, #project-panel-projects, #runtime-panel-projects {

Nit: one selector per line

@@ +16,3 @@
>  }
>  
>  .project-image, .panel-item span {

Nit: one selector per line

@@ +29,5 @@
>  .panel-header {
>    color: #ACACAC;
>    text-transform: uppercase;
>    line-height: 200%;
> +  margin: 5px 5px 0px 5px;

Nit: remove unit for 0 values here and elsewhere

@@ +115,5 @@
> +
> +.runtime-panel-item-wifi,
> +.project-panel-item-openhosted    { background-image: -moz-image-rect(url("icons.png"), 208, 438, 234, 412); }
> +
> +.project-panel-item-newapp, 

Nit: trailing whitespace

@@ +118,5 @@
> +
> +.project-panel-item-newapp, 
> +#runtime-panel-noadbhelper,
> +#runtime-panel-installsimulator   { background-image: -moz-image-rect(url("icons.png"), 234, 438, 260, 412); }
> +

Nit: this appears to be extra blank line at the end of the file, kinda hard to tell for sure in this view.  Only want one blank line at the end.
Attachment #8634478 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8634740 [details] [diff] [review]
WebIDESidebarCSS-v2.patch

Nits fixed.
Attachment #8634478 - Attachment is obsolete: true
Attachment #8634740 - Flags: review?(jryans)
Comment on attachment 8634740 [details] [diff] [review]
WebIDESidebarCSS-v2.patch

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

Still one nit left.

Once you've updated, I'll push this patch to Try.  Should be safe, but doesn't hurt to check.

::: browser/devtools/webide/themes/panel-listing.css
@@ +33,5 @@
>  .panel-header {
>    color: #ACACAC;
>    text-transform: uppercase;
>    line-height: 200%;
> +  margin: 5px 5px 0px 5px;

Nit: remove px from 0 here
Attachment #8634740 - Flags: review?(jryans) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8634756 [details] [diff] [review]
WebIDESidebarCSS-v3.patch

Fixed
Attachment #8634740 - Attachment is obsolete: true
Attachment #8634756 - Flags: review?(jryans)
Comment on attachment 8634756 [details] [diff] [review]
WebIDESidebarCSS-v3.patch

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

Great!

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=060a3b9c5c82
Attachment #8634756 - Flags: review?(jryans) → review+
Try looks good to me, thanks again!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ffb2bb96edc3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
(Assignee)

Updated

3 years ago
Depends on: 1185005
You need to log in before you can comment on or make changes to this bug.