Closed
Bug 1158289
Opened 9 years ago
Closed 9 years ago
Optimize Reader View's line length to have between 45 and 75 characters
Categories
(Firefox :: General, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox38.0.5 | --- | fixed |
firefox39 | --- | fixed |
firefox40 | --- | fixed |
People
(Reporter: mmaslaney, Assigned: bwinton)
References
Details
Attachments
(1 file, 1 obsolete file)
1.30 KB,
patch
|
Margaret
:
review+
bwinton
:
ui-review+
lmandel
:
approval-mozilla-beta+
lmandel
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
Line length/font size info: General wisdom is that 45-75 characters per line is ideal — suggested by Elements of Typographic Style and many other solid resources. This writing goes into explaining both font size proportionally and line width: http://typecast.com/blog/a-more-modern-scale-for-web-typography We utilize this bookmarklet for testing: https://css-tricks.com/bookmarklet-colorize-text-45-75-characters-line-length-testing/
Updated•9 years ago
|
Flags: qe-verify?
Flags: firefox-backlog+
Assignee | ||
Updated•9 years ago
|
Target Milestone: Firefox 38 → Firefox 40
Assignee | ||
Comment 1•9 years ago
|
||
So, this is a terrible patch that shouldn't be checked in, but let me know how you like the behaviour, and I can make a version that doesn't have the terrible bits. ;) There's an OS X build up to play with at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/Width-v1.dmg
Attachment #8598880 -
Flags: ui-review?(mmaslaney)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Points: --- → 2
Updated•9 years ago
|
Flags: qe-verify? → qe-verify-
Reporter | ||
Updated•9 years ago
|
Attachment #8598880 -
Flags: ui-review?(mmaslaney) → ui-review+
Assignee | ||
Comment 2•9 years ago
|
||
Okay, since Michael is happy with this, let's send it over to code review. :)
Attachment #8598880 -
Attachment is obsolete: true
Attachment #8600603 -
Flags: ui-review+
Attachment #8600603 -
Flags: review?(margaret.leibovic)
Comment 3•9 years ago
|
||
Comment on attachment 8600603 [details] [diff] [review] bug1158289.diff Review of attachment 8600603 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/aboutReader.css @@ +53,5 @@ > } > > +#container { > + max-width: 30em; > + margin: 0 auto; Note for posterity: these styles were probably placed on the body originally because we used to not have a #container element. Since this really only needs to apply to the content of the article (not the controls), it's fine to put them on #container.
Attachment #8600603 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8600603 [details] [diff] [review] bug1158289.diff Approval Request Comment [Feature/regressing bug #]: reading mode [User impact if declined]: With large or small font sizes, we'll show too few or too many characters per line. [Describe test coverage new/current, TreeHerder]: Manual [Risks and why]: Low, css-only change. [String/UUID change made/needed]: None.
Attachment #8600603 -
Flags: approval-mozilla-beta?
Attachment #8600603 -
Flags: approval-mozilla-aurora?
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/51177c126be9
Whiteboard: [fixed-in-fx-team]
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/51177c126be9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Comment 8•9 years ago
|
||
This makes the width of the article really really narrow on desktop - there is almost twice as much as whitespace as there is readable content width: http://imgur.com/p3pDyel . This ends up looking really wrong. Is this really the intent here?
Flags: needinfo?(mmaslaney)
Flags: needinfo?(bwinton)
Assignee | ||
Comment 9•9 years ago
|
||
I'mma let Michael answer this one, since he's the designer and all… (But I will note that once we have a reading list sidebar, it won't be as out of proportion.)
Flags: needinfo?(bwinton)
Reporter | ||
Comment 10•9 years ago
|
||
The width should maintain it's 660px max-height, with a min height of 300px for smaller windows.
Flags: needinfo?(mmaslaney)
Reporter | ||
Comment 11•9 years ago
|
||
Regarding our default size, which I'm assuming we may need to enlarge.
Comment 12•9 years ago
|
||
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #10) > The width should maintain it's 660px max-height, with a min height of 300px > for smaller windows. 660px is about double what 30em is for most default font sizes. Should we reopen this?
Flags: needinfo?(bwinton)
Assignee | ||
Comment 13•9 years ago
|
||
I'ld be happy with either re-opening or filing a new bug and assigning it to me. Although I'ld like to hear whether we want 45-75 characters _or_ 660px, because we can't have both at all font sizes… (I mean, we could with crazy letter-spacing, but I don't think we want that. ;)
Flags: needinfo?(bwinton)
Comment 14•9 years ago
|
||
Let us know when it is ready for uplift (not super clear)
Comment 15•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] -- PTO May 9=>16 from comment #14) > Let us know when it is ready for uplift (not super clear) This is ready now. I filed bug 1162873 which got closed as wontfix, so we should just uplift this to 38.0.5 and later.
Flags: needinfo?(ryanvm)
Flags: needinfo?(lmandel)
Updated•9 years ago
|
Flags: needinfo?(ryanvm)
Updated•9 years ago
|
Comment 16•9 years ago
|
||
Comment on attachment 8600603 [details] [diff] [review] bug1158289.diff Aurora is now 40 so approval is not required. I cleared the Aurora request. This change needs to land on m-r (38.0.5) and m-b (39). Approved for both.
Attachment #8600603 -
Flags: approval-mozilla-release+
Attachment #8600603 -
Flags: approval-mozilla-beta?
Attachment #8600603 -
Flags: approval-mozilla-beta+
Attachment #8600603 -
Flags: approval-mozilla-aurora?
Comment 17•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/a65c4683eebc https://hg.mozilla.org/releases/mozilla-release/rev/5fff1e20ed9c
You need to log in
before you can comment on or make changes to this bug.
Description
•