1008116 - Decide on initial icon name & tooltip text for Loop MLP

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

For the toolbar icon, we currently have the label as "Loop" (shown when customizing the icon), and the tooltip text as "Start a conversation".

Are we happy with that for MLP or do we want something different? I suspect Darrin/Romain need to answer this.

Once we have some text, then we can hook it up to l10n for our nightly users.

Comment 1

[Romain Testard [:RT]]

Here are my suggestions
* Button Tooltip: "Invite someone to talk"
* Panel text: "Get a link to invite someone to talk" (trying to be consistent with Phase 1 UX although currently you have to request a link therefore the language is slightly different)

Darrin is that OK with you?

Comment 2

[Darrin Henein [:darrin]]

Sounds reasonable Romain. I wonder if "Get a link *and* invite someone to talk" is better? Pretty much the same thing, just avoids using 'to' twice.

Comment 3

[Romain Testard [:RT]]

I'm good with that!

Comment 4

[Mark Banner (:standard8)]

Split out naming of the icon to bug 1013989.

Comment 5

[Mark Banner (:standard8)]

Attachment: Show the Loop button by default and update the tooltip and panel text.

This fixes this bug and bug 1005065 (they were both simple, so one patch seems best).

We've been told it is ok to add the loop button by default for Nightly.

The label hasn't been decided on yet, but we don't want to expose "Loop" there, so we'll leave it blank for MLP and fix it in bug 1013989.

The loop.properties change of "get_link_to_share" doesn't need an id change as we haven't yet landed this in a repository where L10n is translating.

Comment 6

[Mark Banner (:standard8)]

Attachment: Screen Shot of Panel & Tooltip

Screenshot of the new text & tooltip from the patch.

Comment 7

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

Comment on attachment 8426270 [details] [diff] [review]
Show the Loop button by default and update the tooltip and panel text.

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

Jaws or Gijs, can one of you please have a quick look at this? It looks fine to me, but I know almost nothing about CustomizableUI or default-set

Comment 8

[:Gijs (he/him)]

Comment on attachment 8426270 [details] [diff] [review]
Show the Loop button by default and update the tooltip and panel text.

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

This is completely sane as far as code goes (assuming you've verified that MOZ_LOOP is appropriately defined in all the right places), but you can't change a string (in loop.properties) and not rev the string ID. (not even entirely sure what that hunk is doing in this patch - it seems unrelated)

Comment 9

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

Comment on attachment 8426270 [details] [diff] [review]
Show the Loop button by default and update the tooltip and panel text.

Gijs seems correct that the loop.properties chunk shouldn't be in this patch - it should be in the loop-landing patch.  Given that file isn't already in the tree (and assuming you haven't had l10n love on this yet), it's *probably* OK to keep the same string ID - but as soon as localizers have been let loose on it, it would not be OK to reuse the same ID.

I'm still somewhat concerned about the landing process - we are being asked to review code that is not yet in the tree, but is based on stuff in some git repo somewhere.  But that's how loop is rolling and I trust you to keep the "landing branch" current with only reviewed patches, so a somewhat reluctant r+...

Comment 10

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

Comment on attachment 8426270 [details] [diff] [review]
Show the Loop button by default and update the tooltip and panel text.

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +200,5 @@
>          "bookmarks-menu-button",
>          "downloads-button",
>          "home-button",
> +#ifdef MOZ_LOOP
> +        "loop-call-button",

As this is being added to the default placements, have you run this through tryserver yet (mochitest-bc) to see if this will break any of the customizableui tests?

Comment 11

[Mark Banner (:standard8)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Comment on attachment 8426270 [details] [diff] [review]
> Show the Loop button by default and update the tooltip and panel text.
> 
> Review of attachment 8426270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +200,5 @@
> >          "bookmarks-menu-button",
> >          "downloads-button",
> >          "home-button",
> > +#ifdef MOZ_LOOP
> > +        "loop-call-button",
> 
> As this is being added to the default placements, have you run this through
> tryserver yet (mochitest-bc) to see if this will break any of the
> customizableui tests?

Yep, I'd already tried a subset of tests locally, but tried it in full today and it was all green:

https://tbpl.mozilla.org/?tree=Try&rev=882dc6984a10

Re loop.properties, that's included in here due to comment 1 - the changes are being made together.

Comment 12

[Darrin Henein [:darrin]]

Comment on attachment 8426274 [details]
Screen Shot of Panel & Tooltip

Looks great!

Comment 13

[Mark Banner (:standard8)]

Landed on our reviewed-code-only integration branch:

https://github.com/adamroach/gecko-dev/commit/8a25f204e6e6f8da8ade0688732c9e5b5fd08836

Closing for tracking purposes, I'll comment again with urls as we land this in Mercurial

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/c2b89f2b3a50

Comment 15

[Mark Banner (:standard8)]

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

Comment 16

[Stefan Plewako [:stef]]

Please use meaningful string ids in the future and update them if changing string content.

Comment 17

[u279076]

Verified fixed through previous smoketesting.