Closed
Bug 1344016
Opened 7 years ago
Closed 7 years ago
Update layout of extensions in about:debugging
Categories
(DevTools :: about:debugging, enhancement)
DevTools
about:debugging
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mstriemer, Assigned: mstriemer)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
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
Updated•7 years ago
|
Component: Developer Tools: Debugger → Developer Tools: about:debugging
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review |
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 8•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
(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 12•7 years ago
|
||
mozreview-review |
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 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-review |
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-
Comment 15•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
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 17•7 years ago
|
||
Comment on attachment 8851962 [details]
about_debugging_disabled_links.png
Change color from #6e6e6e to #999999
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8847258 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8847259 -
Attachment is obsolete: true
Assignee | ||
Comment 19•7 years ago
|
||
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)
Comment 20•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
I centred the text and made the divider rgba(0,0,0,0.2) again.
Attachment #8855065 -
Attachment is obsolete: true
Assignee | ||
Comment 23•7 years ago
|
||
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)
Comment 24•7 years ago
|
||
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)
Updated•7 years ago
|
Flags: needinfo?(mstriemer)
Comment 25•7 years ago
|
||
> Do you need me to change your last changeset to try or can you do it?
I meant "push your last changeset", sorry.
Assignee | ||
Comment 26•7 years ago
|
||
I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7e9efeef45f0dbb65f41ccba5270c7ca68b41a to try.
Flags: needinfo?(mstriemer)
Assignee | ||
Comment 27•7 years ago
|
||
Try build looks fine to me so I'm marking this checkin-needed.
Keywords: checkin-needed
Comment 28•7 years ago
|
||
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
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a911151c924
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 30•7 years ago
|
||
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]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•