Use a pretty icon button for toggling the presets list in the CSS Filter tooltip

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: pbro, Assigned: mahdi)

Tracking

unspecified
Firefox 42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Possible icons we can use :
- Pseudo class lock icon (My favourite)
- Sidebar toggle icon
I can't seem to find the lock icon you mention, the sidebar toggle icon looks good btw.
Flags: needinfo?(ntim.bugs)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #2)
> I can't seem to find the lock icon you mention, the sidebar toggle icon
> looks good btw.

This icon : http://hg.mozilla.org/mozilla-central/raw-file/49683d4e9ebd/browser/themes/shared/devtools/images/pseudo-class.svg#pseudo-class

Seems to illustrate pretty well the idea of presets.
Flags: needinfo?(ntim.bugs)
Weird, I can't find the file on my local build, I just updated the repository to make sure, it's not there.

mahdi :: ~/Documents/Workshop/Firefox/browser/themes/shared/devtools/images ~ $ ls | grep pseudo

What am I doing wrong?
Flags: needinfo?(ntim.bugs)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #4)
> Weird, I can't find the file on my local build, I just updated the
> repository to make sure, it's not there.
This icon was added in Firefox 41. So updating was probably not needed

> mahdi :: ~/Documents/Workshop/Firefox/browser/themes/shared/devtools/images
> ~ $ ls | grep pseudo
> 
> What am I doing wrong?

It should be in that directory, not sure what's happening there. Maybe you could try using the Finder/File Explorer instead to find the file. 

Also, have you tried compiling Firefox and checking if the icon is at (0) ? Can you also check if your rule view has this (1) ?

If it's missing in either case, you likely have a problem with your Mercurial/Github setup, you may want to reach people in #introduction on IRC in that case.

(0) : chrome://browser/skin/devtools/pseudo-class.svg#pseudo-class
(1) : https://mdn.mozillademos.org/files/11199/css-lock-hover-1.png
Flags: needinfo?(ntim.bugs)
Hey, sorry for the delay, I couldn't get any help on IRC, did some stuff and found that I had forgot to remove my own .eslintrc after Patrick added an .eslintrc to the repository, it was causing conflicts, thus `hg update` would not work and I had not noticed this as I don't manually run the commands required to update my nightly, I have a bash script, I run it and sleep, usually.

Anyways, here is the patch.

Thanks Tim
Attachment #8638114 - Flags: review?(ntim.bugs)
Comment on attachment 8638114 [details] [diff] [review]
Bug 1184110 - Use a pretty icon button for toggling the presets list in the CSS Filter tooltip; r=ntim

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

I'm not a peer, so my review isn't worth much.

::: browser/devtools/shared/widgets/filter-widget.css
@@ +58,5 @@
>  #container.show-presets .filters-list {
>    width: 300px;
>  }
>  
> +/* The list of filters and list of presets should push their gs to the

Was this change intentional ?
Attachment #8638114 - Flags: review?(ntim.bugs)
Attachment #8638114 - Flags: review?(bgrinstead)
Attachment #8638114 - Flags: review+
Comment on attachment 8638114 [details] [diff] [review]
Bug 1184110 - Use a pretty icon button for toggling the presets list in the CSS Filter tooltip; r=ntim

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

::: browser/devtools/shared/widgets/filter-widget.css
@@ +217,5 @@
>    text-align: center;
>    line-height: 20px;
>  }
>  
> +.add, .presets-icon {

I'd probably just use #toggle-presets here and below and then not add an extra class to the element.  If we need to reuse it later it could be converted to a class

@@ +225,5 @@
>    height: 16px;
>    font-size: 0;
>    vertical-align: middle;
>    cursor: pointer;
> +  margin: 5px;

Should this be `margin: 0 5px` or are you intentionally adding 5px margin to the top and bottom?

@@ +232,5 @@
> +.add {
> +  background: url(chrome://browser/skin/devtools/add.svg);
> +}
> +
> +.presets-icon {

Is there a selector for when the presets list is visible?  if so, we could swap out the background with chrome://browser/skin/devtools/pseudo-class.svg#pseudo-class-checked to show that the section is active.
Attachment #8638114 - Flags: review?(bgrinstead)
Thanks for pointing these out, fixed 'em. Good idea to toggle icon's state between checked/unchecked.
Attachment #8638114 - Attachment is obsolete: true
Attachment #8638267 - Flags: review?(bgrinstead)
Comment on attachment 8638267 [details] [diff] [review]
Bug 1184110 - Use a pretty icon button for toggling the presets list in the CSS Filter tooltip; r=bgrins

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

Looks good to me, can you fix the nit comment?  Then you can reupload the patch with an r+

::: browser/devtools/shared/widgets/filter-widget.css
@@ +217,5 @@
>    text-align: center;
>    line-height: 20px;
>  }
>  
> +.add, #toggle-presets {

Nit: please put this selector on a new line
Attachment #8638267 - Flags: review?(bgrinstead) → review+
I'm not assigned to the bug, can you add me as assignee, please?

Thanks again.
Flags: needinfo?(bgrinstead)
Assignee: nobody → mdibaiee
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
Thanks for the patch !
Keywords: checkin-needed
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #12)
> I'm not assigned to the bug, can you add me as assignee, please?
> 
> Thanks again.

I've asked :dolske to grant you edit-bugs and can-confirm permissions on bugzilla, if you still don't have these permissions by tomorrow, feel free to contact me (email is the best way).
Thanks Tim!
https://hg.mozilla.org/mozilla-central/rev/00bc6fe33be0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.