Quick rule view ... quick layout fix until we have mockups

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

unspecified
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ruleview])

Attachments

(1 attachment, 4 obsolete attachments)

At the moment the layout of the style inspector's rule view is not optimal. I made a few simple tweaks when working on another bug. If we have a patch that is better than the current implementation we should land it.

Of course, when bug 712469 is complete we will be able to use that ... this is more of a 5 minute step in the right direction.
Created attachment 591408 [details] [diff] [review]
Simple patch

Just a simple patch in case the mockups take a long time.
Attachment #591408 - Flags: review?(dao)
Priority: -- → P2
Created attachment 591410 [details] [diff] [review]
Patch
Attachment #591408 - Attachment is obsolete: true
Attachment #591408 - Flags: review?(dao)
Attachment #591410 - Flags: review?(dao)
Comment on attachment 591410 [details] [diff] [review]
Patch

I think this looks pretty good. Will try it out and report back.
Attachment #591410 - Flags: feedback+
Comment on attachment 591410 [details] [diff] [review]
Patch

>--- a/browser/devtools/styleinspector/styleinspector.css
>+++ b/browser/devtools/styleinspector/styleinspector.css

>+.ruleview-firstline {
>+  display: inline-block;
>+}

Replace this with overflow:hidden ...

>--- a/browser/themes/gnomestripe/devtools/csshtmltree.css
>+++ b/browser/themes/gnomestripe/devtools/csshtmltree.css

>+.ruleview-firstline {
>+  width: 100%;
> }

... and drop this?

> .ruleview-rule-source {
>-  background-color: -moz-dialog;
>   padding: 2px 5px;
>+  float: right;
> }

The vertical padding makes this element misaligned. Not sure if this is new due to float:right.

> .ruleview-code {
>-  padding: 2px 5px;
>+  padding: 2px 5px 10px;;
>+}

double semicolon

> .ruleview-enableproperty {
>   height: 10px;
>   width: 10px;
>   -moz-margin-start: 2px;
>   -moz-margin-end: 0;
>+  visibility: hidden;
>+}
>+
>+.ruleview-property:hover > .ruleview-enableproperty {
>+  visibility: visible;
> }

.ruleview-property:not(:hover) > .ruleview-enableproperty {
  visibility: hidden;
}
Attachment #591410 - Flags: review?(dao) → review-
> > .ruleview-enableproperty {
> >   height: 10px;
> >   width: 10px;
> >   -moz-margin-start: 2px;
> >   -moz-margin-end: 0;
> >+  visibility: hidden;
> >+}
> >+
> >+.ruleview-property:hover > .ruleview-enableproperty {
> >+  visibility: visible;
> > }
> 
> .ruleview-property:not(:hover) > .ruleview-enableproperty {
>   visibility: hidden;
> }

This could also be put in the content stylesheet.

Updated

6 years ago
OS: Linux → All
Hardware: x86 → All
Created attachment 591829 [details] [diff] [review]
Simple patch 2

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 591410 [details] [diff] [review]
> Patch
> 
> >--- a/browser/devtools/styleinspector/styleinspector.css
> >+++ b/browser/devtools/styleinspector/styleinspector.css
> 
> >+.ruleview-firstline {
> >+  display: inline-block;
> >+}
> 
> Replace this with overflow:hidden ...
> 

Done

> >--- a/browser/themes/gnomestripe/devtools/csshtmltree.css
> >+++ b/browser/themes/gnomestripe/devtools/csshtmltree.css
> 
> >+.ruleview-firstline {
> >+  width: 100%;
> > }
> 
> ... and drop this?
> 

Done

> > .ruleview-rule-source {
> >-  background-color: -moz-dialog;
> >   padding: 2px 5px;
> >+  float: right;
> > }
> 
> The vertical padding makes this element misaligned. Not sure if this is new
> due to float:right.
> 

It is better without any padding. Also, because there is only a float I have moved it to the content stylesheet.

> > .ruleview-code {
> >-  padding: 2px 5px;
> >+  padding: 2px 5px 10px;;
> >+}
> 
> double semicolon
> 

Fixed

> > .ruleview-enableproperty {
> >   height: 10px;
> >   width: 10px;
> >   -moz-margin-start: 2px;
> >   -moz-margin-end: 0;
> >+  visibility: hidden;
> >+}
> >+
> >+.ruleview-property:hover > .ruleview-enableproperty {
> >+  visibility: visible;
> > }
> 
> .ruleview-property:not(:hover) > .ruleview-enableproperty {
>   visibility: hidden;
> }

Changed

(In reply to Dão Gottwald [:dao] from comment #5)
> > > .ruleview-enableproperty {
> > >   height: 10px;
> > >   width: 10px;
> > >   -moz-margin-start: 2px;
> > >   -moz-margin-end: 0;
> > >+  visibility: hidden;
> > >+}
> > >+
> > >+.ruleview-property:hover > .ruleview-enableproperty {
> > >+  visibility: visible;
> > > }
> > 
> > .ruleview-property:not(:hover) > .ruleview-enableproperty {
> >   visibility: hidden;
> > }
> 
> This could also be put in the content stylesheet.

True, done.
Attachment #591410 - Attachment is obsolete: true
Attachment #591829 - Flags: review?(rcampbell)
Attachment #591829 - Flags: feedback?(dao)
Whiteboard: [ruleview] → [ruleview][has-patch]
Attachment #591829 - Flags: review?(rcampbell) → review+

Comment 7

6 years ago
This wordwraps really badly for me when the rule view is thinner, I get the css location (filename:line) *inside* the rule.

Comment 8

6 years ago
Comment on attachment 591829 [details] [diff] [review]
Simple patch 2

(f- on that basis, but rcampbell can override me)
Attachment #591829 - Flags: feedback-
yeah, this was a little weird for me during playtesting, but overall I think the improvements are "better" overall. Really long selectors can get a little funky in a tiny sidebar. Maybe change the min-width on the sidebar to force more space? Maybe start hiding filenames if below a certain threshold?

Comment 10

6 years ago
If the selectors could just wrap under the filename instead of above, that would be nice.
I can always move the layout into a table ... that is by far the best way to solve the problem.

Comment 12

6 years ago
Let's save large reworkings for bug 712496.
Created attachment 592674 [details] [diff] [review]
Fixed wrapping

(In reply to Dave Camp (:dcamp) from comment #10)
> If the selectors could just wrap under the filename instead of above, that
> would be nice.

It did look a little funky so I have fixed it.
Attachment #591829 - Attachment is obsolete: true
Attachment #591829 - Flags: feedback?(dao)
Status: NEW → ASSIGNED
Whiteboard: [ruleview][has-patch] → [ruleview][land-in-fx-team]
Created attachment 593065 [details] [diff] [review]
Only hide checkboxes

We decided against moving the filenames to the right because of text mangling.

This is just a simple patch to hide the checkboxes ... Dão has already checked it.
Attachment #592674 - Attachment is obsolete: true
Attachment #593065 - Flags: review?(rcampbell)
Attachment #593065 - Flags: review?(rcampbell)

Updated

6 years ago
Attachment #593065 - Flags: review+

Comment 15

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/f5709d1da2ae
Whiteboard: [ruleview][land-in-fx-team] → [ruleview][fixed-in-fx-team]
Attachment #591829 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f5709d1da2ae
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [ruleview][fixed-in-fx-team] → [ruleview]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.