Closed
Bug 1012806
Opened 11 years ago
Closed 11 years ago
Add @media rule shortcuts to sidebar of style editor
Categories
(DevTools :: Style Editor, defect)
DevTools
Style Editor
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)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8425740 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
Comment 6•11 years ago
|
||
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+
Comment 7•11 years ago
|
||
Also, going to have a play.
Assignee | ||
Updated•11 years ago
|
Attachment #8428408 -
Flags: review?(pbrosset)
Comment 8•11 years ago
|
||
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
Comment 9•11 years ago
|
||
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.
Comment 10•11 years ago
|
||
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.
Comment 11•11 years ago
|
||
Rebase onto bug 1013887
Attachment #8428408 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
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.
Comment 13•11 years ago
|
||
(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.
Assignee | ||
Comment 14•11 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 15•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Comment 16•10 years ago
|
||
Added in the release notes: "Developer Tools: New sidebar which displays a list of shortcuts to every @media rule in the current stylesheet"
relnote-firefox:
--- → 33+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•