Closed Bug 1012806 Opened 6 years ago Closed 6 years ago

Add @media rule shortcuts to sidebar of style editor

Categories

(DevTools :: Style Editor, defect)

defect
Not set

Tracking

(relnote-firefox 33+)

RESOLVED FIXED
Firefox 32
Tracking Status
relnote-firefox --- 33+

People

(Reporter: harth, Assigned: harth)

References

Details

Attachments

(2 files, 3 obsolete files)

When working with media queries the other day I really wished there was an easy way to see all the @media rules I had and which ones applied.

I think it could be nice to add a list of the @media rules in each stylesheet to the sidebar, each one's label would be the conditionText of the rule and clicking on it would jump to that stylesheet/line. The ones that currently apply could be highlighted and the ones that don't could be greyed out.

This wouldn't be useful for every stylesheet. I think bootstrap.css has hundreds of @media rules. We'd definitely need a way to limit what we show and collapse the rules.
This is a work in progress with a bad UI, but the functionality in place. It pulls out the media rules from each stylesheet, lists them under the stylesheet name in the sidebar, greys them out if they don't apply, and clicking on them should take you to the sheet/line for that rule (this is buggy though).
Assignee: nobody → fayearthur
Today Gabriel and I mulled the thought of a sidebar to the right of a stylesheet that displays the @media rules and is hidden when there are none.

The original idea was to show the rules under the stylesheet name on the left, but I might like this better.
Attachment #8425740 - Attachment is obsolete: true
Patch to simply add a sidebar.

To enable it, right click on the style editor stylesheet list and select "Show @media sidebar". It appears on stylesheets with @media rules, and is hidden on ones without.
Attachment #8428306 - Attachment is obsolete: true
Attachment #8428408 - Flags: review?(pbrosset)
Attachment #8428408 - Flags: review?(jwalker)
Comment on attachment 8428408 [details] [diff] [review]
Show @media rules in a separate sidebar

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

This is all trivia.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +160,3 @@
>     */
> +  _updateContextMenu: function() {
> +    let smapsString = "showOriginalSources";

Minor: maybe srcTypeString is more obvious than smapsString?

@@ +215,5 @@
>        let href = this.selectedEditor.styleSheet.href;
>        let {line, ch} = this.selectedEditor.sourceEditor.getCursor();
>  
>        this._styleSheetToSelect = {
> +        stylesheet: href,

FWIW, I ended up using url rather than href because it's more general to <style> elements. Not sure that you need to change it, just explaining my thinking.

@@ +594,5 @@
> +
> +    let deferred = promise.defer();
> +    let self = this;
> +
> +    this.on("editor-added", function onAdd(e, selected) {

Do we need self?

    let onAdd = (e, selected) => { ... };
    this.on("editor-added", onAdd);

We might even be able to do:

    return this.once("editor-added").then(selected => {
      if (selected == editor) {
        return editor.details;
      }
    });

@@ +724,5 @@
> +        sidebar.hidden = true;
> +      }
> +      else {
> +        sidebar.hidden = false;
> +      }

Maybe

    sidebar.hidden = (!showSidebar || rules.length === 0);

Also, does that mean you can toggle the sidebar and it will silently not show up for sheets without media rules? That sounds like it might be confusing.

::: toolkit/devtools/server/actors/stylesheets.js
@@ +325,5 @@
> +    } catch(e) {
> +    }
> +
> +    if (this.mql) {
> +      this.mql.addListener(this._matchesChange);

Minor: You could move this into the try/catch and save the 'if'

@@ +494,5 @@
> +    }
> +    if (event.rule && event.rule.type == Ci.nsIDOMCSSRule.MEDIA_RULE) {
> +      this._getMediaRules().then((rules) => {
> +        events.emit(this, "media-rules-changed", rules);
> +      })

Missing ';'
Also I assume you know that we don't need (parens) around single params to fat arrows, and you prefer it to have the params?
Attachment #8428408 - Flags: review?(jwalker) → review+
Also, going to have a play.
Attachment #8428408 - Flags: review?(pbrosset)
For a follow-up I think we should move the context menu to being 2 toggle buttons at the bottom of the list of stylesheets for 2 reasons:

* It's more discoverable
* Right clicking is generally context sensitive, and that menu isn't particularly if you right click on a sheet and say "show original" then all the sheets change
We could also make use of getRuleLocation in stylesheets to allow jumping to a line in an inline stylesheet to go to the right place.
There's a clash between this patch and my coverage changes. It's easy to fix with wig, and I have a rebase if needed.
Thanks for the review.

(In reply to Joe Walker [:jwalker] from comment #6)
> We might even be able to do:
> 
>     return this.once("editor-added").then(selected => {
>       if (selected == editor) {
>         return editor.details;
>       }
>     });

The first editor added might not be the right one, so 'once' won't work.

> Also, does that mean you can toggle the sidebar and it will silently not
> show up for sheets without media rules? That sounds like it might be
> confusing.

Yes. Not sure here. I wonder what percentage of stylesheets have @media rules. If it's not many, than it could be annoying to always have an empty bar around.

> ::: toolkit/devtools/server/actors/stylesheets.js
> @@ +325,5 @@
> > +    } catch(e) {
> > +    }
> > +
> > +    if (this.mql) {
> > +      this.mql.addListener(this._matchesChange);
> 
> Minor: You could move this into the try/catch and save the 'if'

I like to keep my try/catches as tight as possible as I've gotten myself into pickles with swallowing unnecessary errors.
(In reply to Heather Arthur [:harth] from comment #12)
> Thanks for the review.
> 
> (In reply to Joe Walker [:jwalker] from comment #6)
> > We might even be able to do:
> > 
> >     return this.once("editor-added").then(selected => {
> >       if (selected == editor) {
> >         return editor.details;
> >       }
> >     });
> 
> The first editor added might not be the right one, so 'once' won't work.
> 
> > Also, does that mean you can toggle the sidebar and it will silently not
> > show up for sheets without media rules? That sounds like it might be
> > confusing.

If there was a toggle like "[ ] Show sidebar for @media rules" then it might always be clear what's happening.
https://hg.mozilla.org/mozilla-central/rev/6472a5ac620d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Depends on: 1024464
Added in the release notes: "Developer Tools: New sidebar which displays a list of shortcuts to every @media rule in the current stylesheet"
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.