871014 - Refine Reader Menu

Status: VERIFIED FIXED

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

Description

[Ian Barlow (:ibarlow)]

Attachment: Mockup

Our text style menu is in pretty rough shape: http://cl.ly/image/2K0A2M2h0G0r

Since we're in the middle of some other refinements to Reader Mode, it would be great to take another pass at polishing the look and interaction of the menu. A mockup is attached here.

Changes:
1. Represent typeface choices more visually, using a sample and the typeface name instead of "sans" and "serif"
2. For text size adjustments, replace the +/- text size with something more visual
3. Adjust the style of active menu items vs inactive to be bold vs light type, instead of a grey background. 
4. Remove the "margins" setting, as that is more customization than we need in Reader Mode. Appropriate type sizes, line spacing and default margins should be plenty here.

Comment 1

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #0)

> 2. For text size adjustments, replace the +/- text size with something more
> visual

We currently support 7 font sizes, but the new mockup has 5. Should we get rid of the current tiniest/largest font size options we have now?

Comment 2

[:Margaret Leibovic]

Attachment: (Part 1) Remove "margins" setting from reader mode menu

Lucas is traveling this week, so I'll let Brian do these reviews :)

Comment 3

[:Margaret Leibovic]

Attachment: (Part 2) Replace +/- font size setting UI with something more visual

I just decided to go with what I suggested earlier (getting rid of smallest/largest sizes). But that's easy to change if Ian doesn't like it.

Comment 4

[:Margaret Leibovic]

Attachment: (Part 3) Represent typeface choices more visually

I also switched around the color scheme settings to match the mockup.

Comment 5

[:Margaret Leibovic]

Attachment: Screenshot

Ian, lemme know what tweaks need to be made!

Comment 6

[Ian Barlow (:ibarlow)]

This looks really awesome. 

I have a few tweaks after which I think we are good to go!

* I think 5 sizes should be sufficient, yes
* Let's make the "Aa" type about 20% bigger
* Make the row for text switching about 20% taller
* Use Charis Regular for the sample instead of bold
* Use Open Sans Light for the sample instead of regular
* Change the colour of the dividers to #C5D0DA
* Let's try Roboto Light for the inactive menu items

Comment 7

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #6)

> * Use Charis Regular for the sample instead of bold

I was making the sample bold when it was selected, but I now realize that's not necessary since the orange underline makes the selection clear.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 3 v2) Represent typeface choices more visually

This address's Ian's feedback.

Comment 9

[:Margaret Leibovic]

Attachment: Screenshot v2

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 749095 [details] [diff] [review]
(Part 2) Replace +/- font size setting UI with something more visual

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

::: mobile/android/chrome/content/aboutReader.js
@@ +88,5 @@
> +  let fontSizeOptions = [
> +    { name: gStrings.GetStringFromName("aboutReader.fontSizeSample"),
> +      value: 1,
> +      linkClass: "font-size1-sample" },
> +    { name: gStrings.GetStringFromName("aboutReader.fontSizeSample"),

Nit: I'd factor out all of these calls to GetStringFromName("aboutReader.fontSizeSample") into a variable.

Comment 11

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #10)
> Comment on attachment 749095 [details] [diff] [review]
> (Part 2) Replace +/- font size setting UI with something more visual
> 
> Review of attachment 749095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/aboutReader.js
> @@ +88,5 @@
> > +  let fontSizeOptions = [
> > +    { name: gStrings.GetStringFromName("aboutReader.fontSizeSample"),
> > +      value: 1,
> > +      linkClass: "font-size1-sample" },
> > +    { name: gStrings.GetStringFromName("aboutReader.fontSizeSample"),
> 
> Nit: I'd factor out all of these calls to
> GetStringFromName("aboutReader.fontSizeSample") into a variable.

Oh yeah, duh, that's smart.

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 749331 [details] [diff] [review]
(Part 3 v2) Represent typeface choices more visually

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

::: mobile/android/chrome/content/aboutReader.js
@@ +591,5 @@
>          link.classList.add(option.linkClass);
>  
> +      if (option.description !== undefined) {
> +        let description = doc.createElement("div");
> +        description.innerHTML = option.description;

Since the description won't contain HTML, would it be better to just set description.textContent instead of description.innerHTML?

Comment 13

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #12)
> Comment on attachment 749331 [details] [diff] [review]
> (Part 3 v2) Represent typeface choices more visually
> 
> Review of attachment 749331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/aboutReader.js
> @@ +591,5 @@
> >          link.classList.add(option.linkClass);
> >  
> > +      if (option.description !== undefined) {
> > +        let description = doc.createElement("div");
> > +        description.innerHTML = option.description;
> 
> Since the description won't contain HTML, would it be better to just set
> description.textContent instead of description.innerHTML?

I was just following what we did for the link, but yes, you're right. I could also update the link.innerHTML line to do use textContent.

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3143178dc99
https://hg.mozilla.org/integration/mozilla-inbound/rev/0623dece2b89
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfc9340fc48

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3143178dc99
https://hg.mozilla.org/mozilla-central/rev/0623dece2b89
https://hg.mozilla.org/mozilla-central/rev/fbfc9340fc48