:Margaret Leibovic Reporter
	Description • 11 years ago

Follow-up to bug 857989 comment 5.

	Ian Barlow (:ibarlow)
	Comment 1 • 11 years ago

Global changes:

* Reduce title size to 28sp
* Change link colour to blue #00acff 
* Make tapping the title link (vanityfair.com) open the original article
* Change horizontal divider to stop in the middle of the screen
* Change horizontal divider colour / size to #777777 and 1dp thick


Serif-specific changes:

* Only apply serif typeface to titles and content. (leave URL, author / date / caption text in Open Sans)
* Make article title bold

	Mark Capella [:capella] Assignee
	Comment 2 • 11 years ago

Here's a quick put together of everything (I think) ... the only thing I didn't do was with |pressing something to go back something| .... we can already press the back key or click the close-readermode button in the awesomebar to do this ...

So, what am I missing?

	:Margaret Leibovic Reporter
	Comment 5 • 11 years ago

Comment on attachment 742426 [details] [diff] [review]
Quick Hack for feedback

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

Just offering some drive-by feedback...

::: mobile/android/chrome/content/aboutReader.html
@@ +14,5 @@
>      <div id="reader-domain" class="domain"></div>
> +    <div>
> +      <div class="domain-border"></div>
> +    <div>
> +    <br>

I don't really like the fact that we're making another element here just for the border, but I'm not sure how we would otherwise make the border only extend for half of the width of the page. Can we at least try to get rid of this <br> and the <div> that wraps the .domain-border <div>?

::: mobile/android/themes/core/aboutReader.css
@@ +43,5 @@
>  }
>  
> +.domain,
> +.credits {
> +  font-family: "Droid Sans",helvetica,arial,clean,sans-serif;

You should use "Open Sans" instead of "Droid Sans". I'm also not sure we want the other fallbacks besides sans-serif.

@@ +53,5 @@
> +}
> +
> +.domain-border {
> +  border-bottom: 1.5px solid;
> +  border-bottom-color: #777777;

You could combine these two lines into:
border-bottom: 1.5px solid #777777;

@@ +54,5 @@
> +
> +.domain-border {
> +  border-bottom: 1.5px solid;
> +  border-bottom-color: #777777;
> +  float: left;

Is this float necessary?

@@ +60,4 @@
>  }
>  
>  .header > h1 {
> +  font-size: 66px; /* 42px; assume xfactor 1.5 */

We probably want font-size to be in em, if that's what line-height is in.

@@ +252,5 @@
>  /* Image caption text */
>  .content .caption,
>  .content .wp-caption-text,
>  .content figcaption {
> +  font-family: "Droid Sans",helvetica,arial,clean,sans-serif;

Same comment re: "Open Sans".

	Mark Capella [:capella] Assignee
	Comment 6 • 11 years ago

Thanks Margaret!

	Lucas Rocha (:lucasr)
	Comment 7 • 11 years ago

Comment on attachment 742668 [details] [diff] [review]
Patch (v2)

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

Looks good, just need to keep the title style for sans font untouched I think.

::: mobile/android/themes/core/aboutReader.css
@@ +58,5 @@
>  }
>  
>  .header > h1 {
> +  font-size: 2.625em; /* 28sp assume xfactor 1.5 is 42px / 16 yeilds 2.625em */
> +  font-weight: 700; /* bold */

You should only apply these new properties to the ".header > h1" element when reader is using serif fonts. The style for sans uses Open Sans Light and should probably be kept untouched. See screenshot.

I don't consider these comments specially useful to be honest. I'd probably remove them.

@@ +191,5 @@
>  .light > .content a,
>  .light > .content a:visited,
>  .light > .content a:hover,
>  .light > .content a:active {
> +  color: #00acff !important;  /* blue start w-the same */

Comment not very useful...

@@ +198,5 @@
>  .dark > .content a,
>  .dark > .content a:visited,
>  .dark > .content a:hover,
>  .dark > .content a:active {
> +  color: #00acff !important;  /* blue start w-the same */

Ditto.

	Ian Barlow (:ibarlow)
	Comment 8 • 11 years ago

(In reply to Mark Capella [:capella] from comment #3)
> Created attachment 742431 [details]
> Mixed Serif

Couple comments so far Mark:

* Still need to change the link colour to blue
* The article title should be "bold" in the serif view, but "light" in the sans serif view.

Would love to try a build with these changes, when you're ready.

	Mark Capella [:capella] Assignee
	Comment 9 • 11 years ago

Ok, this one with the change to article title font weight based on latest requirement clarifications ... not sure whats left with changing links to blue, that was in the last patch ... are we saying the same thing?

Quick test build:
https://www.dropbox.com/s/i5y1g28bkctkdmq/fennec-23.0a1.en-US.android-arm.apk

	Ian Barlow (:ibarlow)
	Comment 10 • 11 years ago

Hi Mark, couldn't get this build to install. :(

Try uploading again?

	Mark Capella [:capella] Assignee
	Comment 11 • 11 years ago

Odd, loaded and instakked onto my S3 ...

Let;s try to provide it through TRY server
https://tbpl.mozilla.org/?tree=Try&rev=37de5c09c69e

	Ian Barlow (:ibarlow)
	Comment 12 • 11 years ago

Almost there!

* Still need to change the link colour to blue. This seems to be done in content, but not for the one above the article title, so we need to update that.
* The serif article title needs to be set back to bold. Something must have changed there when you flipped the sans-serif one to be Light.

Would love to try a build with these changes, when you're ready.

	Mark Capella [:capella] Assignee
	Comment 13 • 11 years ago

In comment #12 ibarlow mentions "change the link colour to blue ... the one above the article title" ... that's the (now half page width) underline below the page title?

Can you clarify the story here ... it's not an actual link ... we talked about changing it to be one, but I didn't see the case for it ...

	Ian Barlow (:ibarlow)
	Comment 14 • 11 years ago

(In reply to Mark Capella [:capella] from comment #13)
> In comment #12 ibarlow mentions "change the link colour to blue ... the one
> above the article title" ... that's the (now half page width) underline
> below the page title?
> 
> Can you clarify the story here ... it's not an actual link ... we talked
> about changing it to be one, but I didn't see the case for it ...

Sorry, something that got lost in the shuffle at some point... this should indeed be a link, and it should load the webpage that the Reader article is based on. 

Not crucial functionality given that there is also the icon in the title bar, but a nice detail for people who try tapping on it.

	Mark Capella [:capella] Assignee
	Comment 15 • 11 years ago

Ok then, asking anyone for more feedback to make sure I'm clear now on the req's ... later will ask lucasr? ibarlow? for review ...

Summary of changes as I understand the request (and believe this patch provides)

* Reduce article title size to 28sp
* The article title should be "bold" in the serif view, but "light" in the sans serif view

* Change link colours to blue #00acff

* Make tapping the title *link*/domain open the original article
* Change the title *link*/domain horizontal divider to stop in the middle of the screen, and have colour/size of #777777 and 1dp thick

* Only apply serif typeface to article title and content. (leave URL, author / date / caption text in Open Sans)

	Ian Barlow (:ibarlow)
	Comment 16 • 11 years ago

Mark, do you have a build I can try?

	Mark Capella [:capella] Assignee
	Comment 17 • 11 years ago

Combined TRY build for bug 874036 and bug 862445 (also bug 840144 if you want to play with that)... 

builds ok but 2.2 M() and R() testing is failing 2400-sec timeouts and I'm not sure why ... I downloaded from TRY and tested Ok... just fyi for now (?)

https://tbpl.mozilla.org/?tree=Try&rev=b095e4b909ca

	Lucas Rocha (:lucasr)
	Comment 18 • 11 years ago

Comment on attachment 753226 [details] [diff] [review]
Patch (v4)

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

Looks good overall but needs some cleanups on the handling of serif header.

::: mobile/android/chrome/content/aboutReader.js
@@ +375,5 @@
> +    if (this._fontType == "serif") {
> +      titleClasses.add("heavy");
> +    } else {
> +      titleClasses.remove("heavy");
> +    }

No need to do this in javascript. You can accomplish the same with plain CSS.

@@ +522,5 @@
>      this._messageElement.style.display = "none";
>  
>      this._article = article;
>  
> +    this._domainElement.href = article.url;

Tapping on the domain takes to the original article? Nice.

::: mobile/android/themes/core/aboutReader.css
@@ +71,5 @@
>    margin-bottom: 16px;
>    padding: 0px;
>  }
>  
> +.header > h1 .heavy {

Replace the js code for handling 'heavy' with something like:

.serif > .header > h1 {
  font-weight: 700;
}

	Mark Capella [:capella] Assignee
	Comment 19 • 11 years ago

Coooool ....

This addresses those last couple nits ... pushed to TRY looking good also: https://tbpl.mozilla.org/?tree=Try&rev=81b3e9d004b2

	Lucas Rocha (:lucasr)
	Comment 20 • 11 years ago

Comment on attachment 753768 [details] [diff] [review]
Patch (v5)

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

Looks good.

	Mark Capella [:capella] Assignee
	Comment 21 • 11 years ago

on to inbound after a bump .... https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb7f64f278e

	Phil Ringnalda (:philor)
	Comment 22 • 11 years ago

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