Closed Bug 1344016 Opened 7 years ago Closed 7 years ago

Update layout of extensions in about:debugging

Categories

(DevTools :: about:debugging, enhancement)

Product:
DevTools
Component:
about:debugging
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mstriemer, Assigned: mstriemer)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Bug 1344016 - Update styling of about:debugging for add-ons
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
about-debugging.png
69.77 KB, image/png
Details 
There is a new design for the about:debugging add-ons tab since we are adding more options to this page it will get cluttered quickly.

The current options that are available now should be updated to the new layout so that more options can be added.

Mocks: https://mozilla.invisionapp.com/share/RUA0FEJSW#/screens/220703851
Component: Developer Tools: Debugger → Developer Tools: about:debugging
Attached video reload-button-states.mov (obsolete) — 
Emanuela, a few questions (unfortunately the invision doc doesn't seem to let me zoom or show colours):

1. What are the colours for the buttons and divider?
2. What should the hover/click interaction be? I made it the same as the link up top in the attached video (colours too).
3. Is there a shadow on the box?
Flags: needinfo?(emanuela)
Hi Mark,

excellent questions! I mix the answer a bit, but it should answer all your doubts. 


#1 Colours of different links/buttons states

normal, :visited {color: #0087ff} 
:hover, :focus {color: #0052cc, text-decoration: underline}
:active { color: #003399}
:disabled {color: #6e6e6e }

It could be nice to add some transition between the different states.


#2 Divider colour

rgba(0, 0, 0, 0.2)


#3 Box Shadow

Yes, there is!

box-shadow: 0 0 1px rgba(0,0,0,0.12);


That's because I was thinking it could be nice to add a smooth hover effect that transform that box shadow in something just a bit heavier [box-shadow: 0 1px 2px rgba(0,0,0,0.24);] but I'm not too sure now. 


----

Btw, it looks already quite ok! Great job, Mark!
Flags: needinfo?(emanuela)
Attached video about-debugging-styles.mov (obsolete) — 
Here's how it looks with the updated styles. I'll have the patch up soon, let me know how it looks, Emanuela.
Attachment #8846825 - Attachment is obsolete: true
Flags: needinfo?(emanuela)
Comment on attachment 8847259 [details]
Bug 1344016 - Use variables for updated styles

https://reviewboard.mozilla.org/r/120278/#review122186

::: commit-message-7bf94:1
(Diff revision 1)
> +Bug 1344016 - Use variables for updated styles r?jdescottes

This commit updates the styles to use CSS variables. I saw they were used in other parts of this page and I would have used variables in SCSS so I thought I'd give it a shot.

It probably doesn't improve readability since they're so verbose which is too bad but it would prevent tweaking one value and having the alignment go off.
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review122188

::: devtools/client/aboutdebugging/aboutdebugging.css:200
(Diff revision 1)
>  
>  .error-page .error-page-details {
>    color: gray;
>  }
> +
> +.AddonTarget {

I'm not sure what sort of class name convention is used here (I see they're all lowercase right now though) so I went with what we used on addons-frontend where you include the component name as a prefix.

Happy to change it to something else though if that's more consistent.
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review123036

Thanks for the patch Mark!

Tests are failing due to classname and markup changes, so this needs to be fixed.
Let's try to stick to classes already used in the other categories as much as possible.

In the long run we can't really have one category using this new style while the worker & tabs categories are using another one.

::: devtools/client/aboutdebugging/aboutdebugging.css:200
(Diff revision 1)
>  
>  .error-page .error-page-details {
>    color: gray;
>  }
> +
> +.AddonTarget {

Let's stick to lowercase classnames AddonTarget-* -> addon-target-*

::: devtools/client/aboutdebugging/aboutdebugging.css:202
(Diff revision 1)
>    color: gray;
>  }
> +
> +.AddonTarget {
> +  background: #fff;
> +  box-shadow: 0 0 1px rgba(0,0,0,0.12);

nit: consistency: add spaces after commas rgba(0, 0 ,0, 0.12)

::: devtools/client/aboutdebugging/aboutdebugging.css:210
(Diff revision 1)
> +  padding: 4px 16px;
> +  transition: box-shadow 150ms;
> +}
> +
> +.AddonTarget:hover {
> +  box-shadow: 0 1px 2px rgba(0,0,0,0.24);

nit: consistency: add spaces after commas rgba(0, 0 ,0, 0.24)

::: devtools/client/aboutdebugging/components/addons/target.js:65
(Diff revision 1)
>  
>      return dom.li(
> -      { className: "target-container", "data-addon-id": target.addonID },
> +      { className: "AddonTarget", "data-addon-id": target.addonID },
> +      dom.div({ className: "AddonTarget-header" },
> -      dom.img({
> +        dom.img({
> -        className: "target-icon",
> +          className: "AddonTarget-icon",

Why not keep the target-icon class here? 

The only property that needs to be updated is the margin-inline-end. We could simply have a rule :

.addon-target .target-icon {
  margin-inline-end: var(--AddonTarget-padding);
}

instead of duplicating all the rules for .target-icon

::: devtools/client/aboutdebugging/components/addons/target.js:69
(Diff revision 1)
> -      dom.img({
> +        dom.img({
> -        className: "target-icon",
> +          className: "AddonTarget-icon",
> -        role: "presentation",
> +          role: "presentation",
> -        src: target.icon
> +          src: target.icon
> -      }),
> +        }),
> -      dom.div({ className: "target" },
> +        dom.span({ className: "AddonTarget-name", title: target.name }, target.name)

our tests rely on the .target-name selector. I think you could just reuse the styles from our existing .target-name class and add your additional styles with .addon-target .target-name

You can run our tests by doing ./mach test devtools/client/aboutdebugging/test

::: devtools/client/aboutdebugging/components/addons/target.js:72
(Diff revision 1)
> -      }),
> +        }),
> -      dom.div({ className: "target" },
> +        dom.span({ className: "AddonTarget-name", title: target.name }, target.name)
> -        dom.div({ className: "target-name", title: target.name }, target.name)
>        ),
>        dom.button({
> -        className: "debug-button",
> +        className: "AddonTarget-debug-button AddonTarget-button",

our tests rely on the .debug-button, .reload-button selectors. Can we keep non addon specific classnames here?

You can run our tests by doing ./mach test devtools/client/aboutdebugging/test
Attachment #8847258 - Flags: review?(jdescottes)
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review123036

Would you like me to update the worker and tabs categories in this patch? Looks like they would fit the mocks [1] fairly well.

[1] https://mozilla.invisionapp.com/share/RUA0FEJSW#/screens/220706957
(In reply to Mark Striemer [:mstriemer] from comment #3)
> Created attachment 8847222 [details]
> about-debugging-styles.mov
> 
> Here's how it looks with the updated styles. I'll have the patch up soon,
> let me know how it looks, Emanuela.

Hi Mark, it looks great to me! 
One minor: the divider seems a bit too dark from the video, but I don't mind :)

Awesome :)
Flags: needinfo?(emanuela)
Comment on attachment 8848640 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/121528/#review124864

Tests are now passing, thanks :)

Can you please fold this with the other patches.
Attachment #8848640 - Flags: review?(jdescottes) → review+
Comment on attachment 8847258 [details]
Bug 1344016 - Update styling of about:debugging for add-ons

https://reviewboard.mozilla.org/r/120274/#review124866

I'll r+ this one once it's folded with your last patch.
Comment on attachment 8847259 [details]
Bug 1344016 - Use variables for updated styles

https://reviewboard.mozilla.org/r/120278/#review123052

In this particular case I'd prefer to avoid variables. As you commented, it's too verbose to be easily understood. 
Since this stylesheet is only used in one screen at the moment I think testing for UI regressions should be straightforward.
Attachment #8847259 - Flags: review?(jdescottes) → review-
Attached image about_debugging_disabled_links.png (obsolete) — 
I was originally very confused about this screen. 

When all the links are disabled, the gray color used for disabled links feels too dark to really tell the user they are disabled. Probably because there are no enabled links next to it to give a point of comparison. I was hovering and clicking for a few seconds before I remembered that I had to click on the checkbox above the list to enable addon debugging.

What do you think about this?
Attachment #8851962 - Flags: feedback?(emanuela)
Hey Julian,

thank you for the screenshot. I think you're right here.

Let's use #999 as a color for the disabled link.
Comment on attachment 8851962 [details]
about_debugging_disabled_links.png

Change color from #6e6e6e to #999999
Attachment #8847258 - Attachment is obsolete: true
Attachment #8847259 - Attachment is obsolete: true
Attached image about-debugging.png (obsolete) — 
I lightened the divider from rgba(0,0,0,0.2) to rgba(0,0,0,0.1) and it looks much closer to the mocks. Also lightened the disabled button colour.

CSS variables have been removed and commits were folded into one.

Attached a screenshot of how it looks now.
Attachment #8847222 - Attachment is obsolete: true
Attachment #8851962 - Attachment is obsolete: true
Attachment #8851962 - Flags: feedback?(emanuela)
Hi Mark,

I like how it looks, just afraid it may be too light for not HD screens :)

The name of the extensions seems a bit uncentered. Do we have a stanging or something? Where I can see it in action? It could be something simple like setting a different line-height.
Attached image about-debugging.png — 
I centred the text and made the divider rgba(0,0,0,0.2) again.
Attachment #8855065 - Attachment is obsolete: true
We're you wanting to look this over again, Julian? I see that reviewboard is still showing r+ even though the commit updated.
Flags: needinfo?(jdescottes)
Reviewboard will keep forwarding the r+ as soon as it's been given once for a changeset. 
If the changes are significant enough to require a new review, you have to reset the flag yourself in bugzilla :)

In this case I think it's ok to proceed. 
Do you need me to change your last changeset to try or can you do it?
Flags: needinfo?(jdescottes)
Flags: needinfo?(mstriemer)
> Do you need me to change your last changeset to try or can you do it?

I meant "push your last changeset", sorry.
Try build looks fine to me so I'm marking this checkin-needed.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3a911151c924
Update styling of about:debugging for add-ons r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3a911151c924
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I have reproduced this issue with Nightly 54.0a1 (2017-03-02) on Windows 10, 64 bit!

The fix is now verified on Latest Nightly 55.0a1

Build ID 	20170412030252
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170412]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: