Closed Bug 1023279 Opened 10 years ago Closed 10 years ago

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

Categories

(Hello (Loop) :: Client, defect, P1)

Product:
Hello (Loop)
Component:
Client
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: flod, Assigned: standard8)

Details

(Whiteboard: [p=1])

Attachments

(2 files, 1 obsolete file)

Make conversation window title localizable, and add missing license header v2
6.20 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
Screenshot of new title
13.23 KB, image/png
dhenein
: ui-review+
Details
Assignee: nobody → standard8
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
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)
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+
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
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)
Attached image 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+
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
Closed: 10 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.

Attachment

General

Creator:
Created:
Updated:
Size: