Last Comment Bug 720980 - Quick rule view ... quick layout fix until we have mockups
: Quick rule view ... quick layout fix until we have mockups
Status: RESOLVED FIXED
[ruleview]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-25 04:05 PST by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2012-02-07 11:22 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple patch (5.46 KB, patch)
2012-01-25 04:08 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch (5.33 KB, patch)
2012-01-25 04:12 PST, Michael Ratcliffe [:miker] [:mratcliffe]
dao+bmo: review-
rcampbell: feedback+
Details | Diff | Review
Simple patch 2 (4.01 KB, patch)
2012-01-26 09:28 PST, Michael Ratcliffe [:miker] [:mratcliffe]
dcamp: feedback-
Details | Diff | Review
Fixed wrapping (3.75 KB, patch)
2012-01-30 05:52 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Only hide checkboxes (671 bytes, patch)
2012-01-31 05:57 PST, Michael Ratcliffe [:miker] [:mratcliffe]
paul: review+
Details | Diff | Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-25 04:05:01 PST
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.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-25 04:08:23 PST
Created attachment 591408 [details] [diff] [review]
Simple patch

Just a simple patch in case the mockups take a long time.
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-25 04:12:29 PST
Created attachment 591410 [details] [diff] [review]
Patch
Comment 3 Rob Campbell [:rc] (:robcee) 2012-01-25 06:54:31 PST
Comment on attachment 591410 [details] [diff] [review]
Patch

I think this looks pretty good. Will try it out and report back.
Comment 4 Dão Gottwald [:dao] 2012-01-25 08:28:49 PST
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;
}
Comment 5 Dão Gottwald [:dao] 2012-01-25 08:46:38 PST
> > .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.
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-26 09:28:15 PST
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.
Comment 7 Dave Camp (:dcamp) 2012-01-28 15:01:02 PST
This wordwraps really badly for me when the rule view is thinner, I get the css location (filename:line) *inside* the rule.
Comment 8 Dave Camp (:dcamp) 2012-01-28 15:01:44 PST
Comment on attachment 591829 [details] [diff] [review]
Simple patch 2

(f- on that basis, but rcampbell can override me)
Comment 9 Rob Campbell [:rc] (:robcee) 2012-01-29 09:46:47 PST
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 Dave Camp (:dcamp) 2012-01-29 09:59:13 PST
If the selectors could just wrap under the filename instead of above, that would be nice.
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-29 15:53:57 PST
I can always move the layout into a table ... that is by far the best way to solve the problem.
Comment 12 Dave Camp (:dcamp) 2012-01-29 19:55:39 PST
Let's save large reworkings for bug 712496.
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-30 05:52:32 PST
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.
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-31 05:57:20 PST
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.
Comment 16 Tim Taubert [:ttaubert] 2012-02-07 11:22:45 PST
https://hg.mozilla.org/mozilla-central/rev/f5709d1da2ae

Note You need to log in before you can comment on or make changes to this bug.