Closed Bug 1193674 Opened 4 years ago Closed 4 years ago

If room context/name is unavailable, the title of the standalone is displayed as "{{roomName}} - Firefox Hello"

Categories

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

defect

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.1 - Oct 5
Tracking Status
firefox44 --- fixed

People

(Reporter: standard8, Assigned: mancas)

Details

(Whiteboard: [standalone])

Attachments

(1 file, 2 obsolete files)

If room context is unavailable, then we're currently displaying the page title as "{{roomName}} - Firefox Hello".

This can be reproduced by taking a room url and dropping everything after the #.
Rank: 35
Priority: P2 → P3
Assignee: nobody → b.mcb
After an offline conversation with Pau, if there is no context available, the title of the standalone should be "Firefox Hello"
Comment on attachment 8664688 [details] [diff] [review]
Bug 1193674 - If room context/name is unavailable, the title of the standalone is displayed as "{{roomName}} - Firefox Hello"

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

Just to note, I think the patch has the wrong commit message. Possibly a mistake when uploading it?

This patch also needs a test for the new code path. If you look in standaloneRoomViews_test.js, there's already a test for the title in the StandaloneRoomView describe, so you should be able to base it on that, but feel free to ping me if you need help.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +401,5 @@
>            clientShortname: mozL10n.get("clientShortname2")
> +        });
> +
> +        if (!nextState.roomName && !this.state.roomName) {
> +          title = mozL10n.get("clientShortname2");

I wonder if it might be slightly nicer overall to do:

var roomName = nextState.roomName || this.state.roomName;

if (roomName) {
  this.setTitle(....);
} else {
  this.setTitle(mozL10n.get("clientShortname2");
}

That way we're not doing two calculations in the fallback case, and its clearer to read as to what the decision is based on.
Attachment #8664688 - Flags: review?(standard8)
I've added the required tests to the patch Mark. Also I took into account your code style suggestions. Could you review it? :)

Thanks!
Comment on attachment 8665339 [details] [diff] [review]
Bug 1193674 - If room context/name is unavailable, the title of the standalone is displayed as "{{roomName}} - Firefox Hello"

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

Looks great, thanks.
Attachment #8665339 - Flags: review?(standard8) → review+
Keywords: checkin-needed
seems this failed to apply:

adding 1193674 to series file
renamed 1193674 -> Bug-1193674---If-room-contextname-is-unavailable-t.patch
applying Bug-1193674---If-room-contextname-is-unavailable-t.patch
patching file browser/components/loop/test/standalone/standaloneRoomViews_test.js
Hunk #1 FAILED at 144
1 out of 1 hunks FAILED -- saving rejects to file browser/components/loop/test/standalone/standaloneRoomViews_test.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1193674---If-room-contextname-is-unavailable-t.patch
Flags: needinfo?(b.mcb)
Keywords: checkin-needed
Minor issue related to an existing variable name, it is solved now.

Thanks Carsten
Flags: needinfo?(b.mcb)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/53fbb6d6f8d3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.1 - Oct 5
You need to log in before you can comment on or make changes to this bug.