Refine Reader Menu

VERIFIED FIXED in Firefox 24

Status

()

Firefox for Android
Theme and Visual Design
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: ibarlow, Assigned: Margaret)

Tracking

unspecified
Firefox 24
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 748227 [details]
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)

Updated

5 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 1

5 years ago
(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?
(Assignee)

Comment 2

5 years ago
Created attachment 749094 [details] [diff] [review]
(Part 1) Remove "margins" setting from reader mode menu

Lucas is traveling this week, so I'll let Brian do these reviews :)
Attachment #749094 - Flags: review?(bnicholson)
(Assignee)

Comment 3

5 years ago
Created attachment 749095 [details] [diff] [review]
(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.
Attachment #749095 - Flags: review?(bnicholson)
(Assignee)

Comment 4

5 years ago
Created attachment 749096 [details] [diff] [review]
(Part 3) Represent typeface choices more visually

I also switched around the color scheme settings to match the mockup.
Attachment #749096 - Flags: review?(bnicholson)
(Assignee)

Comment 5

5 years ago
Created attachment 749097 [details]
Screenshot

Ian, lemme know what tweaks need to be made!
(Reporter)

Comment 6

5 years ago
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
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 749331 [details] [diff] [review]
(Part 3 v2) Represent typeface choices more visually

This address's Ian's feedback.
Attachment #749096 - Attachment is obsolete: true
Attachment #749096 - Flags: review?(bnicholson)
Attachment #749331 - Flags: review?(bnicholson)
(Assignee)

Comment 9

5 years ago
Created attachment 749332 [details]
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+
(Assignee)

Comment 11

5 years ago
(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+
(Assignee)

Comment 13

5 years ago
(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.

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Updated

5 years ago
Depends on: 876187
(Assignee)

Updated

3 years ago
Depends on: 1124479
You need to log in before you can comment on or make changes to this bug.