Closed Bug 930566 Opened 7 years ago Closed 7 years ago

Change sans serif title to bold

Categories

(Firefox for Android :: Reader View, defect)

26 Branch
x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox28 --- verified

People

(Reporter: ibarlow, Assigned: arephase)

References

Details

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

Attachments

(2 files, 4 obsolete files)

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
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
Whiteboard: [mentor=margaret][lang=css][good first bug]
Can I get assigned this bug? I'm a first timer, so any info about getting set up etc would be helpful. :)
(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
Assignee: nobody → khalidofislam
Can only one person be assigned to a bug at a time? I'm also interested in this.
Can I get this bug assigned? also any information regarding set up and other things would be helpful.I am interested in this.Thanks
(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.
I believe I had some extra spaces in my previous patch, they should be removed now.
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.
Attachment #826486 - Flags: feedback+
Attached patch Combine CSS (obsolete) — Splinter Review
Thanks for the feedback! I've combined the CSS as you suggested.
Attachment #826814 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #826814 - Flags: review?(lucasr.at.mozilla) → feedback+
Attached patch Rebased (obsolete) — Splinter Review
Woops, it should be correct now.
Attachment #826484 - Attachment is obsolete: true
Attachment #826486 - Attachment is obsolete: true
Attachment #826814 - Attachment is obsolete: true
Attachment #826825 - Flags: review?(lucasr.at.mozilla)
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 :-)
Attachment #826825 - Flags: review?(lucasr.at.mozilla) → feedback+
Attached patch More fixesSplinter Review
My apologies... should have checked that everything was good to go instead of having you do so. Hopefully this is the last time? :D
Attachment #826825 - Attachment is obsolete: true
Attachment #826833 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 826833 [details] [diff] [review]
More fixes

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

Nice, looks good ;-)
Attachment #826833 - Flags: review?(lucasr.at.mozilla) → review+
Dominic, please add a checkin-needed keyword to this bug.
Do I have to be assigned to the bug to add it? I can't edit that field.
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.
Flags: needinfo?(arephase)
Here's an on device screenshot of Reader as requested.
Attachment #827430 - Flags: feedback?(ibarlow)
Flags: needinfo?(arephase)
Ian, what do you think?
Flags: needinfo?(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!
Flags: needinfo?(ibarlow)
https://hg.mozilla.org/integration/fx-team/rev/418f757b1dcd
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=css][good first bug] → [mentor=margaret][lang=css][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/418f757b1dcd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=css][good first bug][fixed-in-fx-team] → [mentor=margaret][lang=css][good first bug]
Target Milestone: --- → Firefox 28
Status: RESOLVED → VERIFIED
Comment on attachment 827430 [details]
Reader with bold Clear Sans title

Oops, forgot to + this
Attachment #827430 - Flags: feedback?(ibarlow) → feedback+
Assignee: khalidofislam → arephase
Duplicate of this bug: 877783
You need to log in before you can comment on or make changes to this bug.