Closed Bug 871014 Opened 7 years ago Closed 7 years ago

Refine Reader Menu

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 24

People

(Reporter: ibarlow, Assigned: Margaret)

References

Details

Attachments

(5 files, 2 obsolete files)

Attached image 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.
Assignee: nobody → 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?
Lucas is traveling this week, so I'll let Brian do these reviews :)
Attachment #749094 - Flags: review?(bnicholson)
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.
Attachment #749095 - Flags: review?(bnicholson)
I also switched around the color scheme settings to match the mockup.
Attachment #749096 - Flags: review?(bnicholson)
Attached image Screenshot (obsolete) —
Ian, lemme know what tweaks need to be made!
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
(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.
This address's Ian's feedback.
Attachment #749096 - Attachment is obsolete: true
Attachment #749096 - Flags: review?(bnicholson)
Attachment #749331 - Flags: review?(bnicholson)
Attached image Screenshot v2
Attachment #749097 - Attachment is obsolete: true
Attachment #749094 - Flags: review?(bnicholson) → review+
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.
Attachment #749095 - Flags: review?(bnicholson) → review+
(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 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?
Attachment #749331 - Flags: review?(bnicholson) → review+
(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.
Status: RESOLVED → VERIFIED
Depends on: 876187
Depends on: 1124479
You need to log in before you can comment on or make changes to this bug.