Closed Bug 720980 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: miker, Assigned: miker)

Details

(Whiteboard: [ruleview])

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Simple patch (obsolete) — Splinter Review
Just a simple patch in case the mockups take a long time.
Attachment #591408 - Flags: review?(dao)
Priority: -- → P2
Attached patch Patch (obsolete) — Splinter Review
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.
OS: Linux → All
Hardware: x86 → All
Attached patch Simple patch 2 (obsolete) — Splinter Review
(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+
This wordwraps really badly for me when the rule view is thinner, I get the css location (filename:line) *inside* the rule.
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?
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.
Let's save large reworkings for bug 712496.
Attached patch Fixed wrapping (obsolete) — Splinter Review
(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]
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)
Attachment #593065 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/f5709d1da2ae
Whiteboard: [ruleview][land-in-fx-team] → [ruleview][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f5709d1da2ae
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [ruleview][fixed-in-fx-team] → [ruleview]
Target Milestone: --- → Firefox 13
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.