Closed Bug 929127 Opened 11 years ago Closed 11 years ago

Update the inspector sidebar tabs design according to shorlander's mockups

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: me, Assigned: me)

References

Details

(Whiteboard: [lang=css][mentor=paul][good first verify])

Attachments

(3 files, 15 obsolete files)

(deleted), image/png
Details
(deleted), patch
paul
: review+
Details | Diff | Splinter Review
(deleted), image/png
Details
Attached image Inspector tabs mockup (deleted) —
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release) Build ID: 20130911155223
Blocks: 916766
Component: Untriaged → Developer Tools: Framework
OS: Linux → All
Hardware: x86_64 → x86
Component: Developer Tools: Framework → Developer Tools: Inspector
Whiteboard: [lang=css][mentor=paul]
Summary: Update the inspector tabs design according to shorlander's mockups → Update the inspector sidebar tabs design according to shorlander's mockups
Assignee: nobody → aljullu
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hi, I am new to Bugzilla and am interested in fixing this as my first bug. Please could you assign this to me and point my in the right direction.
Liam, Albert is already looking at this. Sorry.
First attempt. This patch is only for Linux. I took the colors from bug 916766 comment 8 and 9 but I think it does not look like the mockup. Is there something I am missing? In addition, I don't know the colors for :hover and :active, I kept the same colors only removing the gradients. Waiting for your feedback.
Attachment #823519 - Flags: feedback?(paul)
Attached image Screenshot patch 1.png (obsolete) (deleted) —
(In reply to Albert Juhé from comment #3) > Created attachment 823519 [details] [diff] [review] > Bug 929127 - Update the inspector sidebar tabs design according to > shorlander's mockups; r=paul > > First attempt. This patch is only for Linux. > > I took the colors from bug 916766 comment 8 and 9 but I think it does not > look like the mockup. Is there something I am missing? Which colors from the palette did you use? You didn't find anything close to the mockup?
(In reply to Paul Rouget [:paul] from comment #5) > > Which colors from the palette did you use? You didn't find anything close to > the mockup? I took this colors: Background tab bar: #252c33 (from bug 916766 comment 9) Color tab bar: #f5f7fa (from bug 916766 comment 9) Separator: hsl(212,8%,38%) (I used a color picker) Background tab selected: #1d4f73 (from bug 916766 comment 9) Background tab selected (hover): no change Background tab selected (active): no change For what you say, I understand I should ignore the comment in bug 916766 and use only the palette and the mockup, so the colors would be: Background tab bar: hsl(211,24%,27%) (from palette) Color tab bar: hsl(210,30%,85%) (no change) Separator: hsl(213,4%,74%) (from palette) Background tab selected: hsl(205,74%,45%) (from palette) Background tab selected (hover): no change Background tab selected (active): no change I will try to build a patch today or tomorrow with the new colors.
(In reply to Albert Juhé from comment #6) > For what you say, I understand I should ignore the comment in bug 916766 and > use only the palette and the mockup, so the colors would be: To be honest, I don't know. I'll make sure we have a static list of colors asap. Sorry :/
Can you use this: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors You should use the "Toolbars" color. The "Tab Toolbar" is for the top level tabs.
Attached patch Patch 2 (obsolete) (deleted) — Splinter Review
New patch with the new colors. I also changed the background-position of the tabs (I think in my last patch it was not correctly set), however, I couldn't test it on rtl.
Attachment #823519 - Attachment is obsolete: true
Attachment #823519 - Flags: feedback?(paul)
Attachment #824159 - Flags: feedback?(paul)
Attached image Screenshot patch 2.png (obsolete) (deleted) —
Attachment #823521 - Attachment is obsolete: true
There are two things I would like to mention: 1) I took the separator color from the palette, but I think none of them are the same than in the mockup. IMO in my first patch they looked better so, if you agree, I will recover that color. 2) I didn't change the colors for :hover and :active states of the open tab but I think they don't look good with the new default color. Shall we change it?
Flags: needinfo?(paul)
Albert, the background color is right. It looks different because the main tabs toolbar (above) doesn't use the new color. Also, the separator color looks off as well. Can you true to also change the color of the inspector toolbar (use the "Tab Toolbar" color)? And maybe the main tabs toolbar too (use the same color as the inspector tabs).
Flags: needinfo?(paul)
Attached patch Patch 3 (obsolete) (deleted) — Splinter Review
Ok, I attached the new patch. I think it should be the other way around: "Tab toolbar" color for the main tabs toolbar and "Toolbars" color for the inspector toolbar, shouldn't it?
Attachment #824159 - Attachment is obsolete: true
Attachment #824159 - Flags: feedback?(paul)
Attachment #824714 - Flags: feedback?(paul)
Attached image Screenshot patch 3.png (obsolete) (deleted) —
Attachment #824160 - Attachment is obsolete: true
(In reply to Albert Juhé from comment #14) > Created attachment 824715 [details] > Screenshot patch 3.png Ok - we need to make sure we talk about the same things. In your screenshot, there are 3 parts: - the top tabs, where you see Console, Inspector, … we call that toolbox tabs - the inspector toolbar (the bar the includes the "inspect" button, the breadcrumbs and the search input) - the inspector tabs (Rules, Computed, …) In the inspector toolbar AND the inspector tabs, the background should be the color "Toolbars". Don't change the code of the toolbox tabs. Changing the background color for the inspector toolbar and the inspector tabs might impact the other tools, and I think it's alright.
Attached patch Patch 4 (obsolete) (deleted) — Splinter Review
Wow, sorry for misunderstanding! I attached a new patch, it is like patch 2 but with the new inspector toolbar color.
Attachment #824714 - Attachment is obsolete: true
Attachment #824714 - Flags: feedback?(paul)
Attachment #824789 - Flags: feedback?(paul)
Attached image Screenshot patch 4.png (obsolete) (deleted) —
Attachment #824715 - Attachment is obsolete: true
That looks better. I think the separator color is wrong. Can you try to find a better color from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors ?
Attached image Separator color examples (obsolete) (deleted) —
I made tests with the four more similar colors from the wiki: Chrome Foreground (Text) - Grey: #b6babf ; rgba(182, 186, 191, 1) Code Content (Text) - Light: #b8c8d9 ; rgba(184, 200, 217, 1) Content (Text) - Grey: #8fa1b2 ; rgba(143, 161, 178, 1) Content (Text) - Dark Grey: #5f7387 ; rgba(95, 115, 135, 1) But I am not sure which one to choose. Could you give me a hand?
Flags: needinfo?(paul)
Stephen, can you tell us what is the right color for the tab separators in the inspector tabs? If it's not already mentioned in https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors - can you please add it?
Flags: needinfo?(shorlander)
Flags: needinfo?(paul)
It would be good if we could also get the colors for the :hover and :active state in tabs. Specially for the open tab, given that the previous blue for the normal state and the new blue for the :hover state are quite similar.
To be honest, I like the current tab design better.
(In reply to Paul Rouget [:paul] from comment #20) > Stephen, can you tell us what is the right color for the tab separators in > the inspector tabs? If it's not already mentioned in > https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors - can you > please add it? The color on the selected separator is #2d5b7d The color for the regular grey separators is #5a6169 Or we could try and find a light color with an opacity setting that would work for both.
Flags: needinfo?(shorlander)
Attached patch Patch 5 (obsolete) (deleted) — Splinter Review
Here comes the patch with the new colors. > Or we could try and find a light color with an opacity setting that would work for both. I don't know how to approach this. Any help? Otherwise, can we left it with hard-coded colors?
Attachment #824789 - Attachment is obsolete: true
Attachment #824789 - Flags: feedback?(paul)
Attachment #826910 - Flags: review?(paul)
Attached image Screenshot patch 5.png (obsolete) (deleted) —
Attachment #824791 - Attachment is obsolete: true
Attachment #824840 - Attachment is obsolete: true
Comment on attachment 826910 [details] [diff] [review] Patch 5 Don't use the `background` shorthand, use `background-color`. The left toolbar has an inner shadow that makes it stand out: see the light inner border in the left section here: http://i.imgur.com/LgZUAkm.png - you might want to remove it (not sure if that means removing the show box-shadow or just a section of it). This is only for Linux. Can you do the same modifications for the 3 platforms?
Attachment #826910 - Flags: review?(paul)
Attached patch Patch 6 (obsolete) (deleted) — Splinter Review
Ok, I changed 'background' for 'background-color', removed the box-shadow and made the same changes to osx and windows. However, I couldn't test it on these platforms.
Attachment #826910 - Attachment is obsolete: true
Attachment #828161 - Flags: review?(paul)
Attached image Screenshot patch 6.png (obsolete) (deleted) —
Attachment #826911 - Attachment is obsolete: true
Comment on attachment 828161 [details] [diff] [review] Patch 6 > %filter substitution >-%define smallSeparator linear-gradient(hsla(204,45%,98%,0), hsla(204,45%,98%,.1), hsla(204,45%,98%,0)), linear-gradient(hsla(206,37%,4%,0), hsla(206,37%,4%,.6), hsla(206,37%,4%,0)), linear-gradient(hsla(204,45%,98%,0), hsla(204,45%,98%,.1), hsla(204,45%,98%,0)) >-%define solidSeparator linear-gradient(transparent, transparent), linear-gradient(hsla(206,37%,4%,.6), hsla(206,37%,4%,.7)), linear-gradient(hsla(204,45%,98%,.1), hsla(204,45%,98%,.1)) >+%define smallSeparator linear-gradient(#343c45 15%, #5a6169 15%, #5a6169 85%, #343c45 85%) >+%define solidSeparator linear-gradient(#2d5b7d, #2d5b7d) Don't reuse #343c45 here, use transparent. Do we still need to use 2 background-images for the tabs? Can we use a background-color for the background and a background-image for the border?
Attachment #828161 - Flags: review?(paul)
Blocks: 936421
Attached image Transparent background separators example.png (obsolete) (deleted) —
>Don't reuse #343c45 here, use transparent. I used #343c45 instead of transparent because otherwise when hovering a tab it showed the dark color behind the separator (see right separator in the attached image). >Do we still need to use 2 background-images for the tabs? >Can we use a background-color for the background and a background-image for the border? I could never use background-image with gradient in Firefox. I think it's caused by bug 709587. I created an example in jsfiddle: http://jsfiddle.net/FA9B5/2/ In Chrome it looks ok but in Firefox doesn't.
(In reply to Albert Juhé from comment #30) > Created attachment 829436 [details] > Transparent background separators example.png > > >Don't reuse #343c45 here, use transparent. > I used #343c45 instead of transparent because otherwise when hovering a tab > it showed the dark color behind the separator (see right separator in the > attached image). It's because the background linear gradient goes that far. Stop it before. This should work: + background-size: 100% calc(100% - 1px), 1px 100%; > > >Do we still need to use 2 background-images for the tabs? > >Can we use a background-color for the background and a background-image for the border? What I said above would be impossible if we'd use background-color. So forget about that. > I could never use background-image with gradient in Firefox. I think it's > caused by bug 709587. > > I created an example in jsfiddle: http://jsfiddle.net/FA9B5/2/ In Chrome it > looks ok but in Firefox doesn't. border-image and background-image are different beasts. --- r+ if we you can use transparent instead of #343c45 (or convince me it's not possible :p).
Attached patch Patch 7 (obsolete) (deleted) — Splinter Review
Wow, sorry, I misunderstood you! I've attached a new patch with your proposal: + background-size: 100% calc(100% - 1px), 1px 100%; I had a problem and I am not sure if I solved it properly. Before creating the patch I cloned the code from mozilla-central and I saw that the OSX common.css file I was modifying was a little bit different. Is this one: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/common.css#21 From when I started working on this patch, these lines have been removed: -color: hsl(210,30%,85%); -background-image: url(background-noise-toolbar.png), linear-gradient(#3e4750, #3e4750); -box-shadow: 0 1px 0 hsla(204,45%,98%,.05) inset, -1px 0 0 hsla(204,45%,98%,.05) inset, 0 -1px 0 hsla(204,45%,98%,.05) inset; I added back: +color: hsl(210,30%,85%); +background-color: #343c45; I don't have a Mac so I can't test if it's ok.
Attachment #828161 - Attachment is obsolete: true
Attachment #829436 - Attachment is obsolete: true
Attachment #8335464 - Flags: review?(paul)
Flags: needinfo?(paul)
Attachment #8335464 - Attachment is obsolete: true
Attachment #8335464 - Flags: review?(paul)
Attached patch Patch 8 (deleted) — Splinter Review
Sorry, I uploaded the wrong patch. In this one I set: background-size: calc(100% - 2px) 100%, 1px 100%; Given that background-position: 1px, 0; so we must deduct 2px (one for the separator and another for the initial position).
Attachment #8335477 - Flags: review?(paul)
Attached image Screenshot patch 8.png (deleted) —
Attachment #828162 - Attachment is obsolete: true
Comment on attachment 8335477 [details] [diff] [review] Patch 8 Thank you Albert! Now you can add "checkin-needed" in the keywords field.
Attachment #8335477 - Flags: review?(paul) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [lang=css][mentor=paul] → [lang=css][mentor=paul][fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [lang=css][mentor=paul][fixed-in-fx-team] → [lang=css][mentor=paul]
Target Milestone: --- → Firefox 28
Flags: needinfo?(paul)
Whiteboard: [lang=css][mentor=paul] → [lang=css][mentor=paul][good first verify]
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: