Closed
Bug 1000243
Opened 10 years ago
Closed 10 years ago
Console error and stack trace UI follow up
Categories
(DevTools :: Console, defect)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: bgrins, Assigned: msucan)
References
Details
Attachments
(4 files, 5 obsolete files)
345.27 KB,
image/png
|
Details | |
258.96 KB,
image/png
|
Details | |
242.07 KB,
image/png
|
bgrins
:
ui-review+
|
Details |
18.68 KB,
patch
|
Details | Diff | Splinter Review |
We need to have a few updates to the new styling added to the console for errors and stack traces. https://bugzilla.mozilla.org/show_bug.cgi?id=920116#c26 https://bugzilla.mozilla.org/show_bug.cgi?id=920116#c27 https://bugzilla.mozilla.org/show_bug.cgi?id=920116#c28
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•10 years ago
|
||
Some notes from Bug 920116 1) Text in some lines seems off-center vertically 2) The background seems to extend a few px too far to the left (beyond the solid left border) and not quite far enough to the right (just barely covering the line #s) https://bugzilla.mozilla.org/attachment.cgi?id=8410470 3) We need to do something about the contrast in the dark theme. As Darrin mentioned, we could a) Fine tune the orange/background combo as per bgrins' suggestion to get contrast compliance b) Re-evaluate the dark theme text colors in general, as even on black some of the colors lack strong clarity
Reporter | ||
Comment 2•10 years ago
|
||
The vertical alignment seems to be caused by the margin-top in the following rule: .message > .message-body-wrapper { flex: 1 1 100%; margin-top: 4px; } Could we change this to margin: 2px 0; to balance the vertical spacing?
Reporter | ||
Comment 3•10 years ago
|
||
While we are making changes - in getClassNameForValueGrip, IMO we should set 'undefined' to cm-comment (which is a grey color in both themes). This makes entering random commands in the console nicer since the return value from many of them is undefined, and right now it really stands out (as cm-atom).
Reporter | ||
Comment 4•10 years ago
|
||
Here is the current state of things
Reporter | ||
Comment 5•10 years ago
|
||
Using cm-number
Reporter | ||
Comment 6•10 years ago
|
||
Same as green, but using the new colors for green and red in dark theme. https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors
Reporter | ||
Comment 7•10 years ago
|
||
Using blue-grey from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors
Assignee | ||
Comment 8•10 years ago
|
||
This is what I got for now. background-clip worked, but getting a consistent background for all elements wasnt doable without ugly hacks/changes. As such, I abandoned the background-clip approach. This patch *should* fix horizontal and vertical alignments. Please let me know if something doesnt work on your system. The background color for severity=error and the text color for strings are the values you suggested in bug 920116 comment 24. From the experiments you posted I like the green strings most of all. Which color values did you use? Should we switch to green? Thank you!
Attachment #8411292 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Darrin: this is showing the changes I did in the patch. What do you think? Please also check attachment 8411297 [details], and the experiments Brian did. Thanks!
Attachment #8411299 -
Flags: ui-review?(dhenein)
Reporter | ||
Comment 11•10 years ago
|
||
> From the experiments you posted I like the green strings most of all. Which > color values did you use? Should we switch to green? OK, so for Attachment 8411111 [details] I just replaced cm-string with cm-number in console-output.js and didn't make any CSS changes. For Attachment 8411113 [details] I also replaced cm-string with cm-number, but in addition to this I changed the cm-number / theme-fg-color1 in dark-theme.css to #70bf53, and changed the severity background color to rgba(235, 83, 104, .2) or so. The alpha may have been somewhere between .1 and .2. These colors are from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors. We could also use that new red background color (rgba(235, 83, 104, X)) regardless of what foreground we use if it ends up looking better or having better contrast. I'd like to hear Darrin's opinion on what to go with. Darrin: I can send over more screenshots if you want to see different options also.
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8411292 [details] [diff] [review] bug1000243-1.diff Review of attachment 8411292 [details] [diff] [review]: ----------------------------------------------------------------- Code looks good overall. We've discussed a few small changes on IRC so I'll review the updated patch
Attachment #8411292 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8411299 [details]
screenshot 2 with patch
Darrin said this one was his favorite option on #devtools - we are going to go with this text color for string messages / background color for errors, and stick with the current colors for the light theme
Attachment #8411299 -
Flags: ui-review?(dhenein) → ui-review+
Reporter | ||
Updated•10 years ago
|
Attachment #8411111 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8411113 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8411117 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Updated the patch based on your comments and the IRC chat with you and Darrin. I also included an attempt to better align the icon with the rest of visual elements. Screenshot: https://i.imgur.com/TStpObz.png Please let me know if I missed anything.
Attachment #8411292 -
Attachment is obsolete: true
Attachment #8412668 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 15•10 years ago
|
||
Comment on attachment 8412668 [details] [diff] [review] bug1000243-2.diff Review of attachment 8412668 [details] [diff] [review]: ----------------------------------------------------------------- This is good. I've made one minor note about alignment on the icons in OSX. The alignment thing is not a big deal, and I could pick it up in a follow up if you'd like to do it that way. If my inline suggestion looks OK on your system, we could go with that extra top margin and make adjustments if needed in the future. ::: browser/themes/shared/devtools/webconsole.inc.css @@ +41,3 @@ > padding: 0 4px; > width: 8px; > + height: 1.2em; This is causing icon misalignment on OSX. It actually does work if we add 3px top margin on this element (which matches the 3px margin on message-body-wrapper
Attachment #8412668 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Update to fix alignment on macs, based on the IRC chat. Thanks for the review and help with testing.
Attachment #8412668 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/40abf689af04
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/40abf689af04
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•