1133673 - [ReadingList] Pre-land strings for ReadingList v1

Status: RESOLVED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We're not going to make the nightly->aurora merge cut off for strings, so we'll need to pre-land most of the strings that we want for v1.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Went through this today... awaiting answers/clarification/confirmation on most bugs. My notes are here:
https://etherpad.mozilla.org/readinglist-v1-strings

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Blair, bug 1129537 just closed with these strings:

Reader View and Reading List

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Thank you!

Comment 4

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

The Reading List FTU tour (targeting RC38) will include a set of in-product strings which is currently being finalized in bug 1131112. Is this the appropriate place to track these?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Cory Price [:ckprice] from comment #4)
> The Reading List FTU tour (targeting RC38) will include a set of in-product
> strings which is currently being finalized in bug 1131112. Is this the
> appropriate place to track these?

Yes, it'd be good to track all in one place and land all in one go.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Mass change of ReadingList bugs, moving to their own component. Filter bugspam on the following quote:

“Reading is to the mind what exercise is to the body.”
― Joseph Addison

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

This is held up by having the copy finalized in bug 1131112. Strings from that are not yet included in this patch.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8567618 [details] [diff] [review]
WiP v1

Mark: Could you look at the Sync strings? I'm just feeling a little surprised, after finding three identical sets of strings for the engines.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8567618 [details] [diff] [review]
WiP v1

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

So yeah, this sucks:

* We can leave it out from syncSetup.dtd - this is only used for "legacy sync" setup and (in theory) is no longer used - and in the edge-cases where it is, we will not support readingList.  Maybe add a comment to this file indicating it's deprecated, only for legacy sync and will be soon be removed?

* syncCustomize.dtd is for the "select what to sync" dialog that is optionally shown after configuring new sync.  IMO it sucks that we didn't just reuse sync.dtd there - I'm not sure if that was a "policy" decision or just an easy way forward - but I don't think we should both trying to fix that here.

* sync.dtd is certainly needed.

So drop it from syncSetup.dtd, and if you don't think it's worth fixing syncCustomize to reuse the strings, then it's needed in sync.dtd and syncCustomize.dtd.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mark Hammond [:markh] from comment #9)
> and if you don't think it's worth fixing
> syncCustomize to reuse the strings, then it's needed in sync.dtd and
> syncCustomize.dtd.

I'm not going near that can of worms :P Ideally, this bug is a DONTBUILD.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Updated for sync related comments.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8567791 [details] [diff] [review]
Patch v1

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

I haven't cross-checked then against the UX copy, but they LGTM!

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +846,5 @@
>  <!ENTITY emeNotificationsDontAskAgain.accesskey   "D">
> +
> +<!ENTITY readingList.label                        "Reading List">
> +<!ENTITY readingList.sidebar.commandKey           "R">
> +<!-- Pre-landed string for bug 1124400 -->

not sure these "pre-landed" comments add much value - but up to you

@@ +849,5 @@
> +<!ENTITY readingList.sidebar.commandKey           "R">
> +<!-- Pre-landed string for bug 1124400 -->
> +<!ENTITY readingList.showSidebar.label            "Show Reading List Sidebar">
> +<!-- Pre-landed string for bug 1124153 -->
> +<!ENTITY readingList.sidebar.showMore.label       "Show more">

no ellipsis?

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Leaving open to deal with the remaining strings related to in-product promotion (bug 1131112).

https://hg.mozilla.org/mozilla-central/rev/ed70d2025bee

Comment 14

[Francesco Lodolo [:flod]]

> readingList.sidebar.showMore.tooltip = Show %S more items

This needs a proper plural form, can't work for most locales (also broken for %S=1 in English).

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

*facepalm* Fix quickly landed just before merge, with rs=flod:

https://hg.mozilla.org/mozilla-central/rev/056c556b41b2

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Calling this fixed - I've split the in-product tour strings out to bug 1138738, since that's taking longer than I expected.