1079430 - Land the new Loop strings for Firefox 35

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Comment 1

[Maire Reavy [:mreavy]]

Just a reminder:  The first panel for the FTE tour (shown here: http://cl.ly/image/433q0j2K0o1x) is in-product, so the strings for that ("Firefox Hello -- Join the conversation" and "Get Started") need to be included in this bug.  These are on lines 2 and 5 of this spreadsheet: https://docs.google.com/a/mozilla.com/spreadsheets/d/1dRhHrnjWIM5cWoKKXs71DHJwZ_nIFeTxwg8zyoFKgiA/edit#gid=0

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

> +pref("loop.learnMoreUrl", "");

I left this pref blank for now until we have the link to use for it. I tested this and the email body still works it just shows blank where the URL should be. If you are OK with this I will file a follow-up to get this URL added, but it shouldn't block uplift because it won't break string freeze.

> -    // XXX Bug 1013989 will provide a label for the button

I deleted this line since it doesn't apply for mozilla-central anymore, but bug 1013989 should stay open since it is still needed for Firefox 34 (this bug is only targeting Firefox 35).

> +++ b/browser/locales/en-US/chrome/browser/loop/loop.properties
> +clientShortname2=Firefox Hello

This string didn't exist in loop.properties before, but I went with
using clientShortname2 instead of clientShortname so it would be
consistent with loop.en-US.properties.

remote: You can view the progress of your build at the following URL:
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=765ea07ec66e
remote: Alternatively, view them on TBPL (soon to be deprecated):
remote:   https://tbpl.mozilla.org/?tree=Try&rev=765ea07ec66e

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8501339 [details] [diff] [review]
Patch

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

I didn't have time to review this today so someone else can feel free to review before me.

::: browser/components/loop/MozLoopService.jsm
@@ +1235,5 @@
>     */
>    getStrings: function(key) {
>        var stringData = MozLoopServiceInternal.localizedStrings;
>        if (!(key in stringData)) {
> +        Cu.reportError('No string found for key: ' + key);

While you're touching this you could change it to log.error for logging consistency within the file.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8501339 [details] [diff] [review]
Patch

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

Some of the comments for loop.properties also apply to loop.en-US.properties.

::: browser/components/loop/content/js/panel.jsx
@@ +385,5 @@
>          "callUrl": !this.state.pending
>        });
>        return (
>          <div className="generate-url">
> +          <header>{__("share_link_header_text2")}</header>

This shouldn't change. This is the call-url UI, that isn't changing for rooms.

Additionally the new string here "Invite someone to join you", is for the conversation window.

@@ +393,5 @@
>            <ButtonGroup additionalClass="url-actions">
>              <Button additionalClass="button-email"
>                      disabled={!this.state.callUrl}
>                      onClick={this.handleEmailButtonClick}
> +                    caption={mozL10n.get("share_button_label")} />

Likewise, this is part of the existing UI, so I don't think we should change it, just add the new string.

@@ +398,5 @@
>              <Button additionalClass="button-copy"
>                      disabled={!this.state.callUrl}
>                      onClick={this.handleCopyButtonClick}
>                      caption={this.state.copied ? mozL10n.get("copied_url_button") :
> +                                                 mozL10n.get("copy_url_button_label")} />

Ditto re not changing existing UI.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # Panel Strings
>  
> +clientShortname2=Firefox Hello

I'm not sure if this should actually be localizable. It should definitely get a L10n note saying this is a brand name and don't localize, if we leave it in here.

@@ +251,3 @@
>  ## from legal_text_tos and legal_text_privacy. clientShortname will be replaced
>  ## by the brand name, or fall back to client_shortname_fallback
>  legal_text_and_links3=By using {{clientShortname}} you agree to the {{terms_of_use}} \

Can you change the l10n note for this, to go with the change in panel.jsx, and remove the client_shortname_fallback?

@@ +288,5 @@
> +
> +help.label=Help
> +tour.label=Tour
> +
> +rooms.default_room_name_template=Conversation {{conversationLabel}}

We should add an L10n note here explaining that this is replaced by a number.

I'm vaguely wondering if this needs to be a plural form, but I'm not sure it does.

@@ +293,5 @@
> +rooms.leave_button.label=Leave
> +rooms.list.copy_url.tooltip=Copy Link
> +rooms.list.delete.tooltip=Delete Conversation
> +rooms.list.deleteConfirmation.label=Are you sure?
> +rooms.list.title=Current Conversations

Bug 1074672 is adding a better version of this.

@@ +296,5 @@
> +rooms.list.deleteConfirmation.label=Are you sure?
> +rooms.list.title=Current Conversations
> +rooms.name_this_room.label=Name this conversation
> +rooms.new_room_button.label=Start a Conversation
> +rooms.no_rooms.label=No Current Conversations

Bug 1074672 is adding this.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8501339 [details] [diff] [review]
Patch

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

::: browser/components/loop/standalone/content/l10n/loop.en-US.properties
@@ +92,5 @@
> +rooms.list.delete.tooltip=Delete Conversation
> +rooms.list.deleteConfirmation.label=Are you sure?
> +rooms.list.title=Current Conversations
> +rooms.name_this_room.label=Name this conversation
> +rooms.new_room_button.label=Start a Conversation

I think someone needs to address this inconsistent capitalizing of 'Conversation'.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # Panel Strings
>  
> +clientShortname2=Firefox Hello

+1, I'd be for handling this like we handle the 'Firefox' or 'Nightly' brand name(s)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8502215 [details] [diff] [review]
Patch

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

Looks good. r=Standard8

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/3e9de16c4c17

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e9de16c4c17