Closed Bug 927815 Opened 6 years ago Closed 6 years ago

Update the Infobar design according to shorlander's mockups

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: paul, Assigned: aljullu)

References

(Blocks 1 open bug)

Details

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

Attachments

(4 files, 8 obsolete files)

Whiteboard: [good-first-bug][lang=css][mentor=paul]
Hi Paul, I come from Bug 709740. Yes, I would like to contribute to this bug. It's going to be my first attempt to work on Firefox's code so I would need a little bit of help, if it is possible.

I have already downloaded the mozilla-central code, compiled it in my computer and made some small tests to try.

Regarding to this bug, could you mentor me? I suppose I will have to modify a CSS file (given that I have to change the colors). I checked on /browser/devtools/inspector but there is only one CSS file called inspector.css and it doesn't seem to be where the colors are defined.

Could you give me a hand? Where should I search? Thanks in advance!
Albert,

the 2 files you want to look at are:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/highlighter.css#33
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/highlighter.inc.css#20

The first one is about layout, the second one about the style.

I believe you'll only need to touch the second one.

Come by IRC (#devtools) and ping me (paul) if you have any question.

Thanks!
Assignee: nobody → aljullu
Thank you Paul, now it seems quite straightforward. I have only one problem remaining: in the mockups the colors are different depending on the theme: in the dark one they are lighter than in the light theme, but in the CSS file I can't modify them separately. Do you know if there is some parent class or something similar I can use to differentiate them?

And one last thing, I am not sure which blue in the mockups corresponds with which blue in the pallet. I think it is the most bottom left one, but I prefer to ask. :P
You should use the pref `devtools.theme` to check what theme is used.

But for now, I'd recommend that you start with the dark theme. We'll implement the light theme later.
Attached patch Dark theme color changes (obsolete) — Splinter Review
First attempt. It changes the colors of the infobar according to the dark theme mockups.
Attachment #818561 - Flags: feedback?(paul)
Attached patch darkinfobardesign.patch (obsolete) — Splinter Review
Changed background and removed shadow. I still have to make the separators shorter because now they occupy all the height.
Attachment #818708 - Flags: feedback?
Attachment #818708 - Flags: feedback? → feedback?(paul)
Comment on attachment 818708 [details] [diff] [review]
darkinfobardesign.patch

Looks good.

You removed the highlighter-outline. You should keep this (when I mentioned removing the border, I meant removing the border of the infobar).

You'll also need to remove the separators around the the inspect icon and the dropdown menu.

I think the triangle below the infobar still has borders.

In the next version of your patch, can you combined the 2 patches into one file?
Also, can you attach a screenshot as well?

Thank you!
Attachment #818708 - Flags: feedback?(paul)
Attachment #818561 - Flags: feedback?(paul)
We also need the updated icons (hidpi). Shorlander, can you take care of that?
Flags: needinfo?(shorlander)
Attached patch Attemp 3: new infobar dark theme (obsolete) — Splinter Review
I added back the shadow and removed arrow borders and separators. I also combined all patches in one.
Attachment #818561 - Attachment is obsolete: true
Attachment #818708 - Attachment is obsolete: true
Attachment #819362 - Flags: feedback?
Flags: needinfo?(shorlander)
Attachment #819362 - Flags: feedback? → feedback?(paul)
Attached image Attemp 3: screenshot (obsolete) —
Comment on attachment 819362 [details] [diff] [review]
Attemp 3: new infobar dark theme

># HG changeset patch
># Parent 28fd40f6f59af594399237a7a955919482380be2
># User Albert Juhe <aljullu@gmail.com>
>New dark infobar design.
>
>
>diff --git a/browser/themes/shared/devtools/highlighter.inc.css b/browser/themes/shared/devtools/highlighter.inc.css
>--- a/browser/themes/shared/devtools/highlighter.inc.css
>+++ b/browser/themes/shared/devtools/highlighter.inc.css
>@@ -16,114 +16,95 @@
>   box-shadow: 0 0 0 1px rgba(0,0,0,0.3);
>   outline-color: rgba(255,255,255,0.7);
> }
> 
> /* Highlighter - Node Infobar */
> 
> .highlighter-nodeinfobar {
>   color: hsl(200, 100%, 65%);
>-  border: 1px solid hsla(210, 19%, 63%, .5);
>   border-radius: 3px;
>-  background: linear-gradient(hsl(209, 18%, 30%), hsl(210, 24%, 16%)) no-repeat padding-box;
>+  background: hsl(210, 2%, 22%) no-repeat padding-box;

background-color: hsl(210,2%,22%);

Even though we don't correctly do that in this file, color values should not include spaces before commas.

The colors you use should come from the pallet:
http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/DarkTheme-Color-Palette@2x.png

Not directly from the mockup.

I think the correct colors are:
background: hsl(214,13%,24%)
tag: hsl(352,79%,62%)
id: hsl(103,46%,54%)
class: hsl(200,74%,57%)
(I'm not 100% sure, don't trust me on this. Check by yourself in the palette, and see what colors are closer to the mockup)

Should we have different colors for classes and pseudo classes?
Attachment #819362 - Flags: feedback?(paul)
Attached patch Attemp 4: new infobar dark theme (obsolete) — Splinter Review
That's true, I had a problem with the colors. I verified and copied your values except tag, which I think is purple instead of ping. I also removed the white-spaces as suggested.

Thanks for your reviews!
Attachment #819362 - Attachment is obsolete: true
Attachment #819809 - Flags: feedback?
Attached image Attemp 4: screenshot (obsolete) —
Attachment #819363 - Attachment is obsolete: true
Attachment #819809 - Flags: feedback? → feedback?(paul)
Comment on attachment 819809 [details] [diff] [review]
Attemp 4: new infobar dark theme

># HG changeset patch
># Parent acaef53e42643889992e85e6573e6867d97c86c7
># User Albert Juhe <aljullu@gmail.com>
>New colors


The message here should be:
Bug 927815 - Update the Infobar design according to shorlander's mockups. r=paul

Next round, ask we're getting closer, don't ask me for feedback, but for review.


> html|*.highlighter-nodeinfobar-id {
>-  color: hsl(90, 79%, 52%);
>+  color: hsl(103,46%,54%);
> }


Shorlander confirmed that we should use the pink color, not the purple one (square #4).


> html|*.highlighter-nodeinfobar-pseudo-classes {
>-  color: hsl(20, 100%, 70%);
>+  color: hsl(200,74%,57%);
> }


For some reason, we're missing the classes rule. Can you add:
> html|*.highlighter-nodeinfobar-classes
?

Use the same rule as -pseudo-classes.

> .highlighter-nodeinfobar-container[position="top"] > .highlighter-nodeinfobar,
> .highlighter-nodeinfobar-container[position="overlap"] > .highlighter-nodeinfobar {
>-  box-shadow: 0 1px 0 hsla(0, 0%, 100%, .1) inset;
>+  box-shadow: 0 1px 0 hsla(0,0%,100%,.1) inset;
> }


Any good reason to keep this box-shadow?
Attachment #819809 - Flags: feedback?(paul)
Shorlander has updated the palette. We should use the 3rd square from the left (bottom row). Make sure to force the reload the palette (ctrl/cmd + shift + R):

http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/DarkTheme-Color-Palette@2x.png
Ok, I've just applied the changes you said. I hope it's improving.
Attachment #819809 - Attachment is obsolete: true
Attachment #819859 - Flags: review?(paul)
Attached image Attempt 5 screenshot (obsolete) —
Attachment #819810 - Attachment is obsolete: true
Comment on attachment 819859 [details] [diff] [review]
Attempt 5: new infobar dark theme

> .highlighter-nodeinfobar {
>-  color: hsl(200, 100%, 65%);
>-  border: 1px solid hsla(210, 19%, 63%, .5);
>+  color: hsl(200,100%,65%);

This is the default color used in the infobar.
You might want to use the white from the palett (216,33%,97%)
or the classes blue (200,74%,57%).

> html|*.highlighter-nodeinfobar-tagname {
>-  color: white;
>+  color: hsl(352,79%,62%);
> }

The right value is: 285,100%,75%
This is the 3rd square in the palette.
Make sure to look at the latest version of the palette (force refresh, I was seeing the cached version here).

Once you made these changes, we'll land your patch.
Thank you!
Attachment #819859 - Flags: review?(paul) → review+
Ok, here it is.

> This is the default color used in the infobar.
> You might want to use the white from the palett (216,33%,97%)
> or the classes blue (200,74%,57%).

Not sure which one to choose, is this color used anywhere? I wrote hsl(216,33%,97%).

I will wait for your review.
Attachment #819859 - Attachment is obsolete: true
Attachment #819916 - Flags: review?(paul)
Attached image Attempt 6: screenshot
Attachment #819861 - Attachment is obsolete: true
Comment on attachment 819916 [details] [diff] [review]
Attempt 6: new infobar dark theme

Thank you!
Attachment #819916 - Flags: review?(paul) → review+
Keywords: checkin-needed
Pushed this along with a bunch of other checkin-neededs to try:
https://tbpl.mozilla.org/?tree=Try&rev=2c426f843d08
https://hg.mozilla.org/integration/fx-team/rev/1623dffd15e1
Keywords: checkin-needed
Whiteboard: [good-first-bug][lang=css][mentor=paul] → [good-first-bug][lang=css][mentor=paul][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1623dffd15e1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good-first-bug][lang=css][mentor=paul][fixed-in-fx-team] → [good-first-bug][lang=css][mentor=paul]
Target Milestone: --- → Firefox 27
hmm, there seem to be a glitch, see the active state of the "unlock" button.
I am not sure what do you mean, is the "unlock" button the button with the "Select element with mouse" tooltip?

I don't see any problem with it.
The buttons on the infobar have a "windows classic" hover and active state. I see this on Firefox Nightly. Do you want a screenshot ?
(In reply to ntim007 from comment #28)
> The buttons on the infobar have a "windows classic" hover and active state.
> I see this on Firefox Nightly. Do you want a screenshot ?

Yes please (nightly 27 + aurora 26, to see the difference). What's your OS?
Attached image screenshot.png
My OS is Windows 8.1.
Happens on both buttons.
Blocks: 931834
(In reply to ntim007 from comment #30)
> Created attachment 823373 [details]
> screenshot.png
> 
> My OS is Windows 8.1.
> Happens on both buttons.

Thanks a lot! I filed bug 931834.
Depends on: 936422
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.