"Conversation" is hard-coded in dialog for incoming call, missing license header in .properties file

RESOLVED FIXED in mozilla33

Status

Hello (Loop)
Client
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: flod, Assigned: standard8)

Tracking

unspecified
mozilla33
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=1])

Attachments

(2 attachments, 1 obsolete attachment)

Title of the incoming call window is not localizable.
http://hg.mozilla.org/mozilla-central/file/9dc0ffca10f4/browser/components/loop/content/conversation.html#l8

Also, the .properties file is missing the usual MPL2 license header
http://hg.mozilla.org/mozilla-central/file/059f616ab0f8/browser/locales/en-US/chrome/browser/loop/loop.properties
(Assignee)

Updated

4 years ago
Assignee: nobody → standard8
(Assignee)

Updated

4 years ago
Summary: "Customization" is hard-coded in dialog for incoming call, missing license header in .properties file → "Conversation" is hard-coded in dialog for incoming call, missing license header in .properties file
(Assignee)

Comment 1

4 years ago
Created attachment 8440563 [details] [diff] [review]
Make conversation window title localizable, and add missing license header

This makes the conversation window title localizable, setting it up as per https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming-unknown

I've also added the missing header, and re-arranged the properties file a bit to group items together in a slight more logical way.

I've not re-used the "Incoming call" string as that didn't quite match what we wanted for the title, and we'll be getting rid of it when we re-layout the rest of the window as per the UX spec.
Attachment #8440563 - Flags: review?(nperriault)
Attachment #8440563 - Flags: feedback?(francesco.lodolo)
(Assignee)

Updated

4 years ago
Attachment #8440563 - Flags: ui-review?(dhenein)
Comment on attachment 8440563 [details] [diff] [review]
Make conversation window title localizable, and add missing license header

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

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +15,5 @@
> +unable_retrieve_url=Sorry, we were unable to retrieve a call url.
> +
> +# Conversation Window Strings
> +
> +incoming_call_title=Incoming qCall…

If you fix the typo, is good for me ;-)
Attachment #8440563 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8440563 [details] [diff] [review]
Make conversation window title localizable, and add missing license header

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

LGTM, nits and open questions we possibly want to address before landing though.

::: browser/components/loop/content/conversation.html
@@ -4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/.  -->
>  <html>
>    <head>
>      <meta charset="utf-8">
> -    <title>Conversation</title>

nit: Why not keeping an empty tag here? Automatic adding of a title tag when setting document.title has always seemed a bit too magic to me.

::: browser/components/loop/content/js/conversation.js
@@ +7,5 @@
>  var loop = loop || {};
>  loop.conversation = (function(OT, mozL10n) {
>    "use strict";
>  
> +  var sharedViews = loop.shared.views

Missing semicolon.

@@ -9,5 @@
>    "use strict";
>  
> -  var sharedViews = loop.shared.views,
> -      // aliasing translation function as __ for concision
> -      __ = mozL10n.get;

I agree, this doesn't serve any useful purpose.

@@ +171,5 @@
>      // Do the initial L10n setup, we do this before anything
>      // else to ensure the L10n environment is setup correctly.
>      mozL10n.initialize(window.navigator.mozLoop);
>  
> +    document.title = mozL10n.get("incoming_call_title");

Wouldn't we want a test for that?

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +15,5 @@
> +unable_retrieve_url=Sorry, we were unable to retrieve a call url.
> +
> +# Conversation Window Strings
> +
> +incoming_call_title=Incoming qCall…

Typo.
Attachment #8440563 - Flags: review?(nperriault) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 8440586 [details] [diff] [review]
Make conversation window title localizable, and add missing license header v2

Updated patch. Fixed the 'typo' (that was left in after I was double-checking the patch, oops!). Also fixed the other issues and added tests for the init function (which previously had no tests).
Attachment #8440563 - Attachment is obsolete: true
Attachment #8440563 - Flags: ui-review?(dhenein)
Attachment #8440586 - Flags: ui-review?(dhenein)
Attachment #8440586 - Flags: review?(nperriault)
Comment on attachment 8440586 [details] [diff] [review]
Make conversation window title localizable, and add missing license header v2

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

LGTM
Attachment #8440586 - Flags: review?(nperriault) → review+
Priority: -- → P1
Whiteboard: p=1
(Assignee)

Comment 6

4 years ago
Comment on attachment 8440586 [details] [diff] [review]
Make conversation window title localizable, and add missing license header v2

Just remembered, this ui-review should be a screenshot...
Attachment #8440586 - Flags: ui-review?(dhenein)
(Assignee)

Comment 7

4 years ago
Created attachment 8443373 [details]
Screenshot of new title

Here's the updated title bar. Changing the contents of the window is part of a different bug.
Attachment #8443373 - Flags: ui-review?(dhenein)
Comment on attachment 8443373 [details]
Screenshot of new title

Looks fine... what exactly needs ui-review?
Comment on attachment 8443373 [details]
Screenshot of new title

Text looks great, thanks!
Attachment #8443373 - Flags: ui-review?(dhenein) → ui-review+
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d4ee562bf5
Whiteboard: p=1 → [p=1]
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/f9d4ee562bf5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Untracking for QA. Please needinfo me to request testing.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.