Closed Bug 1016528 Opened 6 years ago Closed 6 years ago

UI for source map and @media sidebar settings

Categories

(DevTools :: Style Editor, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: harth, Assigned: harth)

References

Details

Attachments

(2 files, 2 obsolete files)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1012806#c8, you can toggle "Show original sources" and "Show media sidebar" from the context menu of the style editor stylesheet list, but it's not a good fit.

We should have a separate, global (to the style editor) UI for toggling these prefs.
This removes the context menu and puts the original sources and media sidebar options into a global menu with a settings icon.
Assignee: nobody → fayearthur
Attachment #8430134 - Flags: review?(pbrosset)
Sorry Heather, yesterday was a holiday here. I'll try and get to this review today, if not, on Monday.
Comment on attachment 8430134 [details] [diff] [review]
Move context menu items to an options menu

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

The changes look good to me.

I only have a couple of minor comments about the code.

And I also noticed a CSS bug that occurs when resizing the left sidebar too small. I will attach a screenshot.
If this is going to be a somewhat complex fix that would be better handled in a separate bug, let me know and I'll R+ this one instead.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +142,5 @@
>        this._importFromFile(this._mockImportFile || null, this._window);
>      }.bind(this));
>  
> +    let optionsMenu = this._panelDoc.getElementById("style-editor-options-popup");
> +    optionsMenu.addEventListener("popupshowing",

The couple of 'addEventListener' calls should have associated 'removeEventListener' calls in 'StyleEditorUI.prototype.destroy'

@@ +159,2 @@
>     */
>    _updateContextMenu: function() {

Now that the menu is not a right-click contextual menu, you should perhaps rename this function to something like '_updateOptionsMenu'.
Attachment #8430134 - Flags: review?(pbrosset)
Here's the problem I was mentioning: resize the sidebar so that it's too small to contain the buttons and options cog.

- the buttons do disappear beneath the editor as expected (although there's a small glitch with the editor's left border which is missing where the button is)
- the options icon floats on top of the editor though.

A simple fix is to only make the options icon behave like the button.
A slightly more complex (and I think perhaps better) fix would be to make the buttons responsive and adapt to the available width (elliding the text if needed), until there is really not enough space and they start hiding.
We're going to need space for some buttons for css coverage quite soon. Maybe we should put the cog in a toolbar at the bottom of the file list, where it can be joined by the results of bug 1016288.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> And I also noticed a CSS bug that occurs when resizing the left sidebar too
> small. I will attach a screenshot.
> If this is going to be a somewhat complex fix that would be better handled
> in a separate bug, let me know and I'll R+ this one instead.

Can do a follow-up. This was a problem before with the other buttons on that toolbar. And it's not likely they'll resize it to be that small.
(In reply to Joe Walker [:jwalker] from comment #5)
> We're going to need space for some buttons for css coverage quite soon.
> Maybe we should put the cog in a toolbar at the bottom of the file list,
> where it can be joined by the results of bug 1016288.

Maybe we can do that later, but it'll look weird for this. One thing we could change is the "New" and "Import" buttons taking up a lot of space.
Attachment #8430134 - Attachment is obsolete: true
Attachment #8433510 - Flags: review?(pbrosset)
Attached patch Real patchSplinter Review
The real patch, with the right eventListener call.
Attachment #8433510 - Attachment is obsolete: true
Attachment #8433510 - Flags: review?(pbrosset)
Attachment #8433535 - Flags: review?(pbrosset)
Blocks: 1019848
Comment on attachment 8433535 [details] [diff] [review]
Real patch

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

Changes look good to me. I see my comments addressed, and I'm ok to handle the sidebar resizing part in a follow-up bug (filed follow-up bug 1019848).
Attachment #8433535 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Hi, could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b0df0f26f0a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.