Closed Bug 856803 Opened 9 years ago Closed 9 years ago

Change About:feedback title to Open Sans Light

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: ibarlow, Assigned: nickecarlo)

Details

(Whiteboard: [mentor=margaret][lang=css])

Attachments

(1 file, 3 obsolete files)

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
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.
Whiteboard: [mentor=margaret][lang=css]
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!
Assignee: nobody → piyushkhemka123
OS: Mac OS X → Android
Hardware: x86 → ARM
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 :)
Attachment #736364 - Flags: review?(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?
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.
More in the diff than necessary? Wait I will redo it.
Attachment #736364 - Attachment is obsolete: true
Attachment #736364 - Flags: review?(margaret.leibovic)
Attachment #740568 - Flags: review?(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 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.
Attachment #740568 - Flags: review?(margaret.leibovic) → review-
Piyush, are you still working on this?
Flags: needinfo?(piyushkhemka123)
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.
Flags: needinfo?(piyushkhemka123)
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.
Assignee: piyushkhemka123 → nobody
I'll take this since I am already working on Feedback stuff elsewhere.
Assignee: nobody → 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.
Flags: needinfo?(margaret.leibovic)
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
Yes, looking good! You should address ibarlow's feedback and I can review a patch!
Flags: needinfo?(margaret.leibovic)
Nick, the screenshots you posted in channel, https://dl.dropboxusercontent.com/u/22352304/textsmaller.png, look great. Ship it!
Attached patch Patch for bug 856803 (obsolete) — Splinter Review
- 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).
Attachment #740568 - Attachment is obsolete: true
Attachment #772231 - Flags: review?(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.
Attachment #772231 - Flags: review?(margaret.leibovic) → review+
Addressed the nit above.
Attachment #772231 - Attachment is obsolete: true
Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8508d454d8c5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.