Implement strings for Reader Mode toolbar controls

VERIFIED FIXED in Firefox 38

Status

()

Firefox
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: mmaslaney, Assigned: Margaret)

Tracking

38 Branch
Firefox 38
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

The following strings need to be implemented for Readermode.

Readermode Controls

(X) – "Close Readermode"
(Aa) – "Type Controls"
(+) – "Add to Reading List"
(-) – "Delete from Reading List"
(=) – "Open Reading List" / "Close Reading List"

Readermode Footer

(-) "Delete This Article"
(Assignee)

Comment 1

3 years ago
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #0)

> Readermode Footer
> 
> (-) "Delete This Article"

In bug 1120735 I punted on implementing this footer because I wasn't sure what it's supposed to mean. How is this different than deleting the article from the reading list?

I can file another bug to add this footer, but I want to be clear about what it's supposed to be doing.
Flags: needinfo?(mmaslaney)
(Assignee)

Comment 2

3 years ago
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #0)

> Readermode Controls
> 
> (X) – "Close Readermode"

Also, we should do an audit to make sure we're consistent with either "Readermode" or "Reader Mode". I think that we've generally been using "Reader Mode" (two words).

(NI to antlam to chime in on the consistency aspect)
Flags: needinfo?(alam)
(In reply to :Margaret Leibovic from comment #2)
> (In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from
> comment #0)
> 
> > Readermode Controls
> > 
> > (X) – "Close Readermode"
> 
> Also, we should do an audit to make sure we're consistent with either
> "Readermode" or "Reader Mode". I think that we've generally been using
> "Reader Mode" (two words).
> 
> (NI to antlam to chime in on the consistency aspect)

YES for consistency! Thanks for NI-ing me to this.

I've been following Bugzilla and referring it to Reader Mode (two words) everywhere. :)
Flags: needinfo?(alam)
(Assignee)

Updated

3 years ago
Blocks: 558882
No longer depends on: 558882
Summary: Implement strings for Readermode → Implement strings for Reader Mode toolbar controls
How do we feel about "Reading View", as suggested by Matej. I feel that it creates a nice association with it's buddy feature: "Reading List".
Flags: needinfo?(mmaslaney) → needinfo?(matej)

Comment 5

3 years ago
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #4)
> How do we feel about "Reading View", as suggested by Matej. I feel that it
> creates a nice association with it's buddy feature: "Reading List".

Just added it as an option in bug 1129537.
Flags: needinfo?(matej)
(Assignee)

Comment 6

3 years ago
Created attachment 8564400 [details] [diff] [review]
WIP - Add title text to reader mode toolbar buttons

I have a WIP here to add these strings, and to actually include them with the buttons.

I'll finish this up and request review once we decide on the final strings we're going to use.
(Assignee)

Updated

3 years ago
Depends on: 1132307
(Assignee)

Updated

3 years ago
Depends on: 1129537

Comment 7

3 years ago
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #0)
> The following strings need to be implemented for Readermode.

Is there a UI mockup of these strings that I could reference?

> Readermode Controls
> 
> (X) – "Close Readermode"

Sounds like we already have this one covered, but just in case, it should be "Close Reading View"

It could also be "Exit Reading View"

> (Aa) – "Type Controls"

I'm not sure what this one means or how it would be used.

> (+) – "Add to Reading List"

Looks good.

> (-) – "Delete from Reading List"

Maybe "Remove from Reading List" instead?

> (=) – "Open Reading List" / "Close Reading List"

Great.

> Readermode Footer
> 
> (-) "Delete This Article"

To consistent with the above capitalization, this should be "Delete this Article"
(Assignee)

Comment 8

3 years ago
(In reply to Matej Novak [:matej] from comment #7)
> (In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from
> comment #0)
> > The following strings need to be implemented for Readermode.
> 
> Is there a UI mockup of these strings that I could reference?

These are the strings for the tooltip/accessibility text for the reader mode toolbar buttons.

Here's a mock-up:
https://projects.invisionapp.com/share/XA217X8SY#/screens/57292604

Comment 9

3 years ago
(In reply to :Margaret Leibovic from comment #8)
> (In reply to Matej Novak [:matej] from comment #7)
> > (In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from
> > comment #0)
> > > The following strings need to be implemented for Readermode.
> > 
> > Is there a UI mockup of these strings that I could reference?
> 
> These are the strings for the tooltip/accessibility text for the reader mode
> toolbar buttons.
> 
> Here's a mock-up:
> https://projects.invisionapp.com/share/XA217X8SY#/screens/57292604

Thanks.

I just noticed an inconsistency in what I put in comment 7. Instead of "Delete this Article," let's make it "Remove this Article" so it matches with "Remove from Reading List."
(In reply to Matej Novak [:matej] from comment #7)
> Sounds like we already have this one covered, but just in case, it should be
> "Close Reading View"
> 
> It could also be "Exit Reading View"

bug 1129537 is almost closed on using "Reading View" or "Reader View".
(Assignee)

Comment 11

3 years ago
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #10)
> (In reply to Matej Novak [:matej] from comment #7)
> > Sounds like we already have this one covered, but just in case, it should be
> > "Close Reading View"
> > 
> > It could also be "Exit Reading View"
> 
> bug 1129537 is almost closed on using "Reading View" or "Reader View".

Sounds good. I'll update my patch and post for review.

Most of the strings are shared between desktop and Android, so this should take care of both of them, but I'll also audit the Android strings to make sure everything is consistent.
Reader View and Reading List
(Assignee)

Comment 13

3 years ago
Created attachment 8566272 [details]
MozReview Request: bz://1131303/margaret

/r/4027 - Bug 1131303 - Update strings for reader view/reading list UI, and add title text to reader view toolbar buttons. r=Unfocused,rnewman

Pull down this commit:

hg pull review -r ed6792c9cbf17ccd95ba6e342a0b0d462827b07f
Attachment #8566272 - Flags: review?(rnewman)
Attachment #8566272 - Flags: review?(bmcbride)
(Assignee)

Updated

3 years ago
Attachment #8564400 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/4027/#review3187

Some definite and some potential capitalization issues here.

I note that desktop uses Title Case for menu items etc., which might conflict with Android. In that case we will need to split these string files.

::: browser/locales/en-US/chrome/browser/readerMode.properties
(Diff revision 1)
> -readerMode.exit=Exit Reader Mode
> +readerView.exit=Exit Reader View

Android style guide says to use sentence case whenever possible:

```

* Use sentence-style capitalization for all UI strings: “Words to live by.”

* Capitalize all important words in:
        App names (Calendar, Google Drive)
        Named features (Android Beam, Face Unlock)
        Proper nouns (Statue of Liberty, San Francisco Giants)
        
* Be conservative. Don't capitalize words that aren't part of a formal feature name:
        Sim card lock, Home screen, not Sim Card Lock, Home Screen.
```

Unless we're saying that "Reader View" is a 'named feature' that the user is expected to understand, these should be sentence case.

::: toolkit/locales/en-US/chrome/global/aboutReader.properties
(Diff revision 1)
> +aboutReader.footer.deleteThisArticle=Delete this Article

These should definitely be sentence case:

```
  Type controls
  Delete this article
```

"Reading list" and "reader view" are the same question about _named feature_ as in my first issue.
Attachment #8566272 - Flags: review?(rnewman) → review-
(In reply to Richard Newman [:rnewman] from comment #14)
> https://reviewboard.mozilla.org/r/4027/#review3187
> 
> Some definite and some potential capitalization issues here.

I'm definitely in favor of sentence case as much as possible, and we should follow the respective style guides for all strings. I would, however, consider Reader View and Reading List to be named features. That's the way I've been treating them in the first time use copy, to not only tell users about the functionality, but also to introduce the names of the features.
(Assignee)

Comment 16

3 years ago
Comment on attachment 8566272 [details]
MozReview Request: bz://1131303/margaret

/r/4027 - Bug 1131303 - Update strings for reader view/reading list UI, and add title text to reader view toolbar buttons. r=Unfocused,rnewman

Pull down this commit:

hg pull review -r 43d1803270277fd937e12f787e62fe09aa785e06
Attachment #8566272 - Flags: review- → review?(rnewman)
(Assignee)

Comment 17

3 years ago
I updated "Type controls" and "Delete this article" to sentence case, but I kept "Reader View" and "Reading List" as suggested by Matej.
https://reviewboard.mozilla.org/r/4027/#review3237

Ship It!

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 2)
>    _updateToggleButton: function Reader_updateToggleButton() {

Sidenote: method functions don't need names these days (not for years!), and we can even do this:

```
  _updateToggleButton() {
    ...
  },
```

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 2)
> +  _onList: function Reader_onList() {

s/Reader_onList//
https://reviewboard.mozilla.org/r/4027/#review3245

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 2)
> +    // To be implemented (bug 1132665)

Bug 1133489
Attachment #8566272 - Flags: review?(bmcbride) → review+
Comment on attachment 8566272 [details]
MozReview Request: bz://1131303/margaret

https://reviewboard.mozilla.org/r/4025/#review3247

Ship It!
(Assignee)

Comment 21

3 years ago
(In reply to Richard Newman [:rnewman] from comment #18)
> https://reviewboard.mozilla.org/r/4027/#review3237
> 
> Ship It!
> 
> ::: toolkit/components/reader/AboutReader.jsm
> (Diff revision 2)
> >    _updateToggleButton: function Reader_updateToggleButton() {
> 
> Sidenote: method functions don't need names these days (not for years!), and
> we can even do this:

I won't introduce new ones, but I'll file a mentor bug to get rid of these throughout the tree.

I'm assuming you meant an r+ to apply here :)
Comment on attachment 8566272 [details]
MozReview Request: bz://1131303/margaret

Such reviewboard, wow.
Attachment #8566272 - Flags: review?(rnewman) → review+
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/1146999121f4
(Assignee)

Comment 24

3 years ago
(FYI filed bug 1134940 for the function names issue)
https://hg.mozilla.org/mozilla-central/rev/1146999121f4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
(Assignee)

Updated

3 years ago
Depends on: 1136716
Verified fixed on Nightly 39.0a1 (2015-03-09) and Aurora 38.0a2 (2015-03-09), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: --- → verified
Flags: qe-verify+
QA Contact: andrei.vaida
Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl
Version: Firefox 38 → 38 Branch
(Assignee)

Comment 28

3 years ago
Comment on attachment 8566272 [details]
MozReview Request: bz://1131303/margaret
Attachment #8566272 - Attachment is obsolete: true
Attachment #8619416 - Flags: review+
(Assignee)

Comment 29

3 years ago
Created attachment 8619416 [details]
MozReview Request: Bug 1131303 - Update strings for reader view/reading list UI, and add title text to reader view toolbar buttons. r=Unfocused,rnewman
You need to log in before you can comment on or make changes to this bug.