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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

See bug 1153184 comment 57.

Comment 1

[Tim Nguyen :ntim]

Possible icons we can use :
- Pseudo class lock icon (My favourite)
- Sidebar toggle icon

Comment 2

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

I can't seem to find the lock icon you mention, the sidebar toggle icon looks good btw.

Comment 3

[Tim Nguyen :ntim]

(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.

Comment 4

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

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?

Comment 5

[Tim Nguyen :ntim]

(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

Comment 6

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1184110 - Use a pretty icon button for toggling the presets list in the CSS Filter tooltip; r=ntim

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

Comment 7

[Tim Nguyen :ntim]

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 ?

Comment 8

[Brian Grinstead [:bgrins]]

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.

Comment 9

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1184110 - Use a pretty icon button for toggling the presets list in the CSS Filter tooltip; r=bgrins

Thanks for pointing these out, fixed 'em. Good idea to toggle icon's state between checked/unchecked.

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1184110 - Use a pretty icon button for toggling the presets list in the CSS Filter tooltip; r=bgrins

Thanks Bryan!

Comment 12

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

I'm not assigned to the bug, can you add me as assignee, please?

Thanks again.

Comment 13

[Tim Nguyen :ntim]

Thanks for the patch !

Comment 14

[Tim Nguyen :ntim]

(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).

Comment 15

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Thanks Tim!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/00bc6fe33be0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00bc6fe33be0