930566 - Change sans serif title to bold

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Ian Barlow (:ibarlow)]

Now that Clear Sans has landed, the Light weight doens't really balance well as a title. Let's switch it to bold, as in this mockup: http://cl.ly/image/1X2L220R2y2v

Comment 1

[:Margaret Leibovic]

To fix this bug we'll need to edit this CSS file:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css

A good tool for developing this is the remote inspector. Instructions for setting that up are here:
https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging

Comment 2

[khalidofislam]

Can I get assigned this bug? I'm a first timer, so any info about getting set up etc would be helpful. :)

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to khalidofislam from comment #2)
> Can I get assigned this bug? I'm a first timer, so any info about getting
> set up etc would be helpful. :)

Sure you can! The first step is get Fennec built and running. See instructions here:

  https://wiki.mozilla.org/Mobile/Fennec/Android

Then have a look at the links that Margaret posted in comment #1 above. The fix for this bug is just about updating that CSS file.

I recommend joining our IRC channel so that you can get more immediate help from us. You can find us on the #mobile channel in irc.mozilla.org. For more information, see: https://wiki.mozilla.org/IRC

Comment 4

[Dominic Kuang]

Can only one person be assigned to a bug at a time? I'm also interested in this.

Comment 5

[kumar rishav (:rishav_)]

Can I get this bug assigned? also any information regarding set up and other things would be helpful.I am interested in this.Thanks

Comment 6

[Aaron Train [:aaronmt]]

(In reply to Dominic from comment #4)
> Can only one person be assigned to a bug at a time? I'm also interested in
> this.

(In reply to kumar rishav from comment #5)
> Can I get this bug assigned? also any information regarding set up and other
> things would be helpful.I am interested in this.Thanks

Notes are in comment #3. Seems like this bug is popular. Assignment need not matter; simply post a patch and we'll review it.

Comment 7

[Dominic Kuang]

Attachment: Changes sans-serif heading to bold

Comment 8

[Dominic Kuang]

Attachment: Removes extra spaces in previous patch

I believe I had some extra spaces in my previous patch, they should be removed now.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 826486 [details] [diff] [review]
Removes extra spaces in previous patch

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

Thanks for the patch Dominic! Looks good overall but I think we can simplify the CSS a bit.

ps: for future patches, make sure you request a patch review. Set the review flag to 'r?' and direct the review to me.

::: mobile/android/themes/core/aboutReader.css
@@ +78,5 @@
>    font-weight: 700;
>  }
>  
> +.sans-serif > .header > h1 {
> +  font-weight: 700;

Now that both serif and sans-serif use the same font-weight, it would probably be better to just set 'font-weight: 700' in '.header > h1' and remove the existing 'serif > .header > h1' selector.

Comment 10

[Dominic Kuang]

Attachment: Combine CSS

Thanks for the feedback! I've combined the CSS as you suggested.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 826814 [details] [diff] [review]
Combine CSS

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

Looks good. It seems you sent a patch based on your previous patch. You have to base your changes on top of the repo. Just giving f+ to get the patch rebased.

Comment 12

[Dominic Kuang]

Attachment: Rebased

Woops, it should be correct now.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 826825 [details] [diff] [review]
Rebased

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

You still need to remove the '.serif > .header > h1' selector :-)

Comment 14

[Dominic Kuang]

Attachment: More fixes

My apologies... should have checked that everything was good to go instead of having you do so. Hopefully this is the last time? :D

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 826833 [details] [diff] [review]
More fixes

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

Nice, looks good ;-)

Comment 16

[Lucas Rocha (:lucasr)]

Dominic, please add a checkin-needed keyword to this bug.

Comment 17

[Dominic Kuang]

Do I have to be assigned to the bug to add it? I can't edit that field.

Comment 18

[Lucas Rocha (:lucasr)]

Dominic, just before go ahead with landing this patch, could please share a screenshot of reader with the patch applied? Just so ibarlow (our designer) can have a look.

Comment 19

[Dominic Kuang]

Attachment: Reader with bold Clear Sans title

Here's an on device screenshot of Reader as requested.

Comment 20

[Lucas Rocha (:lucasr)]

Ian, what do you think?

Comment 21

[Ian Barlow (:ibarlow)]

That looks about right. I'd like to try it at different text sizes / headline lengths but let's ship this and fine tune if we need to from there. 

Thanks Dominic!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/418f757b1dcd

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/418f757b1dcd

Comment 24

[Ian Barlow (:ibarlow)]

Comment on attachment 827430 [details]
Reader with bold Clear Sans title

Oops, forgot to + this