856803 - Change About:feedback title to Open Sans Light

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Ian Barlow (:ibarlow)]

We had some issues making this work back when we implemented the feedback solicitation tool. 

Seems like this might be easier, now that we ship Open Sans in our app?

What it looks like now: http://cl.ly/image/1p0W3a3j3w1T
What it should look like (lighter title style): http://cl.ly/image/033m1M1l1x1h/o

Comment 1

[:Margaret Leibovic]

I'm making this into a mentor bug in case someone else has time to get to it before me.

The files for about:feedback are here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.xhtml
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutFeedback.css

It looks like we'll need to change the font styles for the .header rule.

Comment 2

[:Margaret Leibovic]

Piyush reached out to me via email and is working on this bug now.

As always, let me know if you need any more help!

Comment 3

[Piyush]

Attachment: Changed the font to Open Sans Light

Comment 4

[:Margaret Leibovic]

Comment on attachment 736364 [details] [diff] [review]
Changed the font to Open Sans Light

Thanks for the patch! When uploading a patch, you should set the review? flag with an appropriate reviewer. In this case, that's me :)

Comment 5

[:Margaret Leibovic]

Comment on attachment 736364 [details] [diff] [review]
Changed the font to Open Sans Light

There's a lot more in this diff than there should be, and it looks to just be whitespace changes. I can't tell exactly what's going on, but maybe your text editor is changing the line endings?

Comment 6

[:Margaret Leibovic]

Piyush, did you build with this patch to test? I just built this and it doesn't look like it worked to change the font.

Ian, are you sure we ship Open Sans Light? I'm having trouble seeing that anywhere.

Comment 7

[Piyush]

More in the diff than necessary? Wait I will redo it.

Comment 8

[Piyush]

Attachment: Changed the font to OpenSans-Light

Comment 9

[:Margaret Leibovic]

Thanks for the updated patch. However, did you see my question in comment 6? I found that this didn't work when I tested, and I'm wondering if it worked for you.

Comment 10

[:Margaret Leibovic]

Comment on attachment 740568 [details] [diff] [review]
Changed the font to OpenSans-Light

Review of attachment 740568 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry it's taken me a while to get back to this patch! It somehow slipped through the cracks :(

If you post a new patch that addresses these comments, I promise to get back to it more quickly.

::: mobile/android/themes/core/aboutFeedback.css
@@ +25,5 @@
>    display: none;
>  }
>  
>  .header {
> +   font-family: 'OpenSans-Light';

Instead of changing the font family. You should change the font weight, using font-weight: lighter;.

You should also update the font-family style on the body to use "Open Sans". Setting font-family: "Open Sans",sans-serif; should be sufficient.

Comment 11

[:Margaret Leibovic]

Piyush, are you still working on this?

Comment 12

[Piyush]

Uhhh Sorry, I completely forgot about this. Hadn't checked my mail in a while. As of the moment I am not working on it. My university exams are going on.

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

Thanks for the update. Resetting the owner to nobody. If you get some more free time and this is still open feel free to start it back up.

Comment 14

[Nicolas Carlo [:nickecarlo]]

I'll take this since I am already working on Feedback stuff elsewhere.

Comment 15

[Nicolas Carlo [:nickecarlo]]

Before I post the patch, just wanted to see if you think the way it looks with Open Sans is okay or not:

https://dl.dropboxusercontent.com/u/22352304/OpenSans_Lighter.png

The above puts the way it was with the old font and the way it is with Open Sans side by side.

Comment 16

[Ian Barlow (:ibarlow)]

This looks awesome. I think now that it's Light you may want to bump up the text size by about 15% to closer match the original mocks

Comment 17

[:Margaret Leibovic]

Yes, looking good! You should address ibarlow's feedback and I can review a patch!

Comment 18

[Ian Barlow (:ibarlow)]

Nick, the screenshots you posted in channel, https://dl.dropboxusercontent.com/u/22352304/textsmaller.png, look great. Ship it!

Comment 19

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for bug 856803

- Changed the font family to Open Sans ,sans-serif
- Changed text size from 14px to 12px
- And changed fine print size from 12px to 10px
- The settings are the same as the screenshots (link posted above by Ian).

Comment 20

[:Margaret Leibovic]

Comment on attachment 772231 [details] [diff] [review]
Patch for bug 856803

Review of attachment 772231 [details] [diff] [review]:
-----------------------------------------------------------------

If Ian approves of the font-size changes (sounds like he does), then this patch looks good to me!

::: mobile/android/themes/core/aboutFeedback.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  body {
>    -moz-text-size-adjust: none;
> +  font-family: "Open Sans" ,sans-serif;

Nit: Remove the space before the comma here.

Comment 21

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for check-in

Addressed the nit above.

Comment 22

[:Margaret Leibovic]

Thanks!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8508d454d8c5

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8508d454d8c5