891446 - Reader: move title rule down

Status: RESOLVED FIXED

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

Description

[Ian Barlow (:ibarlow)]

Screenshot of current implementation: http://cl.ly/image/3B0M2P2U1Q0k

That rule below the URL should move away from the url text by about 15dp, like in these mocks http://cl.ly/image/2K3s0J2G2B2l

Comment 1

[Nicolas Carlo [:nickecarlo]]

It also looks much better without the www. preceding the URL.

Comment 2

[Ian Barlow (:ibarlow)]

Yes. Bonus points if you remove that too ;)

Comment 3

[Nicolas Carlo [:nickecarlo]]

(In reply to Ian Barlow (:ibarlow) from comment #2)
> Yes. Bonus points if you remove that too ;)

Hehe. If Margaret wants to do both of these things (rule and www., m., etc) in this bug then I'll take it. Otherwise I can file another bug and leave this for someone new since it'll only take one line of code to fix the rule...and I seem to be taking all the easy bugs away from new folks :-/.

Comment 4

[:Margaret Leibovic]

(In reply to Nicolas Carlo [:nickecarlo] from comment #3)
> (In reply to Ian Barlow (:ibarlow) from comment #2)
> > Yes. Bonus points if you remove that too ;)
> 
> Hehe. If Margaret wants to do both of these things (rule and www., m., etc)
> in this bug then I'll take it. Otherwise I can file another bug and leave
> this for someone new since it'll only take one line of code to fix the
> rule...and I seem to be taking all the easy bugs away from new folks :-/.

Filing a separate new bug sounds like a good plan, it's good to keep some easy bugs open for new people.

nickecarlo, if you want, you could even make yourself the mentor for this bug, since you probably know how to fix it! :)

Comment 5

[Nicolas Carlo [:nickecarlo]]

To fix this bug we'd just need to work on the code here https://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#61

Comment 6

[Nicolas Carlo [:nickecarlo]]

(In reply to :Margaret Leibovic from comment #4)
> Filing a separate new bug sounds like a good plan, it's good to keep some
> easy bugs open for new people.
> 
> nickecarlo, if you want, you could even make yourself the mentor for this
> bug, since you probably know how to fix it! :)

Filed bug 892942 for this.

Comment 7

[Sushant Hiray [:sushant]]

@Nicolas: Are we just looking into increasing the padding bottom for .domain in this bug?

Comment 8

[Nicolas Carlo [:nickecarlo]]

(In reply to Sushant Hiray [:sushant] from comment #7)
> @Nicolas: Are we just looking into increasing the padding bottom for .domain
> in this bug?

Hi Sushant,

I would play around with margin-top values for .domain-border and see which value comes closest to the images posted by ibarlow above.

You can post screenshots here for ibarlow to see if you're on the right track, along with a patch, if you like.

Let me know if you want to work on this bug and I'll assign it to you. :-)

Comment 9

[Sushant Hiray [:sushant]]

Nicolas,
I would want to work on this bug but I'm particularly new to Firefox for Android.
I've cloned the mozilla central and have found the file where I would need to edit.
Could you help me in figuring out, how can I compile and check in reader mode?

Comment 10

[Nicolas Carlo [:nickecarlo]]

(In reply to Sushant Hiray [:sushant] from comment #9)
> Nicolas,
> I would want to work on this bug but I'm particularly new to Firefox for
> Android.
> I've cloned the mozilla central and have found the file where I would need
> to edit.
> Could you help me in figuring out, how can I compile and check in reader
> mode?

Awesome. I've assigned the bug to you.

Follow this guide to set up your build environment: https://wiki.mozilla.org/Mobile/Fennec/Android

If you are using an emulator then the mozconfig settings here: https://wiki.mozilla.org/Mobile/Fennec/Android#Setup_Fennec_mozconfig are not going to work (at least they don't for me), let me know and I will help you out with that though.

If you're stuck in the process of setting all of this up, send me an email directly and I'll try to get back to you ASAP.

If you need help faster than an email response, you can always go to #mobile on irc.mozilla.org (details here: https://wiki.mozilla.org/IRC)

So again if you're stuck, don't hesitate to email me directly. :-)

Comment 11

[Sushant Hiray [:sushant]]

Hi Nicolas,
Sorry for the delay. I was caught up in my univ exams.
So here are some screenshots:
Current Version looks like this: http://i.imgur.com/5K1Hjhp.png
A top margin of 15 px looks this way http://i.imgur.com/jUJ4Rje.png
and a margin of 10 px looks this way http://i.imgur.com/tS8cCna.png 

Do tell me which looks good.

Comment 12

[Nicolas Carlo [:nickecarlo]]

(In reply to Sushant Hiray [:sushant] from comment #11)
> Hi Nicolas,
> Sorry for the delay. I was caught up in my univ exams.
> So here are some screenshots:
> Current Version looks like this: http://i.imgur.com/5K1Hjhp.png
> A top margin of 15 px looks this way http://i.imgur.com/jUJ4Rje.png
> and a margin of 10 px looks this way http://i.imgur.com/tS8cCna.png 
> 
> Do tell me which looks good.

Thanks Sushant. Let's see which one Ian likes the best since its his call.

Comment 13

[Ian Barlow (:ibarlow)]

Great stuff -- I prefer the 15px one. Thanks!

Comment 14

[Sushant Hiray [:sushant]]

Attachment: 891446.patch : Moving the title ruler down

I have submitted the patch corresponding to 15px. I hope this looks good :)

Comment 15

[Nicolas Carlo [:nickecarlo]]

Comment on attachment 795894 [details] [diff] [review]
891446.patch : Moving the title ruler down

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

Looks good to me. I've given + as feedback. I'm marking Margaret for review.

Comment 16

[:Margaret Leibovic]

Comment on attachment 795894 [details] [diff] [review]
891446.patch : Moving the title ruler down

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

Looks good to me! Thanks Sushant and Nicolas!

Comment 17

[Sushant Hiray [:sushant]]

Welcome!
Do count me in if you need further help! :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b57d1e7e8762