1083466 - Implement the FTE UI in the panel body with a button to open the tour tab

Status: VERIFIED FIXED

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

Description

[Matthew N. [:MattN]]

See bug 1044994 for some details along with http://cl.ly/image/2r2m1e0x2g0H

The URL of the tab to open/switchTo will need to be agreed upon with www.mozilla.org

See also bug 1074932 for another way to launch the tour with a similar URL.

Comment 1

[Romain Testard [:RT]]

To be clear, there are 3 components which are displayed on FTU:
* "Get started" button opening the tour in a new tab (bug 1083466)
    - This will disappear the next time the user opens the panel
* The links to ToS and privacy notice (bug 1084362)
    - This will disappear after the first time someone joins a room created by the user (this is because there are 2 links and given that the panel closes as you click a link, we need to give an opportunity to the user to open both links)
* The Telefonica logo (bug 1074720)
    - This will disappear the next time the user opens the panel

Comment 3

[Matthew N. [:MattN]]

The URL of the tour page should be in a pref so we can adjust it for testing. The URL still needs to be decided.

Comment 4

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

Have we decided on a URL yet?

Comment 5

[:shell escalante]

hi Cory,  do we know the URL of the tab to open/switchTo for the tour yet?

Comment 6

[Holly Habstritt Gaal [:Habber]]

We have not decided on a URL for this. What is our deadline to do so?

Comment 7

[:shell escalante]

Hi Holly, Hi Cory,

we kinda need the URL's now. Jared can do most of the patch without it, but we'll need it to complete the bug - and if you want to test after this sprint, then sooner is better....

Comment 8

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Hi Shell!

Quick question: if we give you the URL next Friday, what impact would that have on the 25th? Is this bug blocking any of the other bugs listed in bug 1098620?

This tour is unique, so some of the previous conventions we use don't quite apply.

Thanks for the reply!

Comment 9

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

(In reply to Cory Price [:ckprice] from comment #8)
> Hi Shell!
> 
> Quick question: if we give you the URL next Friday, what impact would that
> have on the 25th? Is this bug blocking any of the other bugs listed in bug
> 1098620?
> 
> This tour is unique, so some of the previous conventions we use don't quite
> apply.
> 
> Thanks for the reply!

Why will it take until next Friday to get the URL? The webpage doesn't need to be fully functional for us to check in the URL. Is there a delay due to some discussions about domain names/partners? Or is it heavily tied to the functionality of the webpage?

Comment 10

[Holly Habstritt Gaal [:Habber]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> (In reply to Cory Price [:ckprice] from comment #8)
> > Hi Shell!
> > 
> > Quick question: if we give you the URL next Friday, what impact would that
> > have on the 25th? Is this bug blocking any of the other bugs listed in bug
> > 1098620?
> > 
> > This tour is unique, so some of the previous conventions we use don't quite
> > apply.
> > 
> > Thanks for the reply!
> 
> Why will it take until next Friday to get the URL? The webpage doesn't need
> to be fully functional for us to check in the URL. Is there a delay due to
> some discussions about domain names/partners? Or is it heavily tied to the
> functionality of the webpage?

We need to put some thought into where this should live on mozilla.org. Also, there are cases where this URL may be access from a snippet or other page, so we need to consider all the ways this URL might be used, so that we get it right. Apologies for not yet having this discussion amongst our team. We were not aware of a deadline for this.

Comment 11

[Justin Dolske [:Dolske]]

In the meantime, let's just land a placeholder URL, and file a followup to change it to the final URL.

I'd suggest https://people.mozilla.org/~jwein/comingsoon.html with content "coming soon, bug xxx". Or perhaps just https://people.mozilla.org/~dolske/tmp/rickroll.ogg ;).

Comment 12

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Justin Dolske [:Dolske] from comment #11)
> In the meantime, let's just land a placeholder URL, and file a followup to
> change it to the final URL.

This sounds like a great solution. Again, we hope to have the final URL sometime next week. Sorry if this has caused any delay on your end.

> I'd suggest https://people.mozilla.org/~jwein/comingsoon.html with content
> "coming soon, bug xxx". Or perhaps just
> https://people.mozilla.org/~dolske/tmp/rickroll.ogg ;).

rickroll +1

Comment 13

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

Attachment: Patch

Comment 14

[Matthew N. [:MattN]]

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

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

::: browser/app/profile/firefox.js
@@ +1644,5 @@
>  pref("loop.enabled", true);
>  pref("loop.server", "https://loop.services.mozilla.com");
>  pref("loop.seenToS", "unseen");
> +pref("loop.gettingStartedSeen", false);
> +pref("loop.gettingStartedUrl", "https://people.mozilla.com/~dolske/tmp/rickroll.ogg");

Nit: loop.gettingStarted.seen & loop.gettingStarted.url

::: browser/components/loop/MozLoopAPI.jsm
@@ +459,5 @@
> +     *
> +     * Any errors thrown by the Mozilla pref API are logged to the console
> +     * and cause false to be returned.
> +     */
> +    setLoopBoolPref: {

Thanks for adding this

@@ +612,5 @@
>          return MozLoopService.openFxASettings();
>        },
>      },
>  
> +    openGettingStarted: {

I think this should have tour in the name so it's more obvious we can also use it for the gear menu tour item. e.g. openTourTab.

::: browser/components/loop/MozLoopService.jsm
@@ +1375,5 @@
>        log.error("Error opening FxA settings", ex);
>      }
>    }),
>  
> +  openGettingStarted: Task.async(function() {

ditto

::: browser/components/loop/content/js/panel.jsx
@@ +166,5 @@
> +
> +    render: function() {
> +      var gettingStartedSeen = navigator.mozLoop.getLoopBoolPref("gettingStartedSeen");
> +      if (gettingStartedSeen) {
> +        return <div />;

Why not `null`?

::: browser/components/loop/content/shared/css/panel.css
@@ +116,5 @@
> +  text-align: center;
> +  margin-bottom: .5em;
> +}
> +
> +.button-uitour {

Why the fte & getstarted mentions for some and then very different terminology "uitour" for this? uitour has other meanings already so I don't think we should overload it.

Comment 15

[Justin Dolske [:Dolske]]

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

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

::: browser/app/profile/firefox.js
@@ +1644,5 @@
>  pref("loop.enabled", true);
>  pref("loop.server", "https://loop.services.mozilla.com");
>  pref("loop.seenToS", "unseen");
> +pref("loop.gettingStartedSeen", false);
> +pref("loop.gettingStartedUrl", "https://people.mozilla.com/~dolske/tmp/rickroll.ogg");

Kudos for actually doing the rickroll. :) Alas that was meant as humor (too subtle), and I don't think we should actually do this. Please change this to something like the other, actual, suggestion in comment 11.

Comment 16

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

Attachment: Patch v1.1

Addressed the issues from Matt's review. I got the humor of dolske's recommendation (using rickroll), but I'm not sure what a different type of coming-soon page would accomplish, since this is temporary anyways. I've left it unchanged :)

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 8523597 [details] [diff] [review]
Patch v1.1

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

> Addressed the issues from Matt's review. I got the humor of dolske's
> recommendation (using rickroll), but I'm not sure what a different type of
> coming-soon page would accomplish, since this is temporary anyways. I've
> left it unchanged :)

A button that loads a joke video with no explanation is going to confuse and/or annoy Nightly users, and then we have to deal with the bugs they file. This "what URL should we use" saga is already too long.

Change this URL per last comments. I suppose you could alternatively set it to the followup bug itself, "https://bugzilla.mozilla.org/show_bug.cgi?id=1099462".

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8523597 [details] [diff] [review]
Patch v1.1

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +459,5 @@
> +     *
> +     * Any errors thrown by the Mozilla pref API are logged to the console
> +     * and cause false to be returned.
> +     */
> +    setLoopBoolPref: {

I already dislike the fact that we don't have a `MozLoopService.getPref()` function that uses `getPrefType`, but we can have that here:

`setPref: function(prefName, value, [prefType]){}` with an optional preftype for when you know that the pref isn't set in firefox.js (rare)

How does that sound?

::: browser/components/loop/MozLoopService.jsm
@@ +1375,5 @@
>        log.error("Error opening FxA settings", ex);
>      }
>    }),
>  
> +  openGettingStartedTour: Task.async(function() {

why Task.async when you're not passing in a generator?

::: browser/components/loop/content/js/panel.jsx
@@ +168,5 @@
> +      var gettingStartedSeen = navigator.mozLoop.getLoopBoolPref("gettingStarted.seen");
> +      if (gettingStartedSeen) {
> +        return null;
> +      }
> +      navigator.mozLoop.setLoopBoolPref("gettingStarted.seen", true);

this pref setter should go in a `componentDidMount` function

@@ +172,5 @@
> +      navigator.mozLoop.setLoopBoolPref("gettingStarted.seen", true);
> +      return (
> +        <div id="fte-getstarted">
> +          <header id="fte-title">
> +            {__("first_time_experience_title", {

nit: please use `mozL10n.get()`; we're trying to migrate away from `__()`

@@ +173,5 @@
> +      return (
> +        <div id="fte-getstarted">
> +          <header id="fte-title">
> +            {__("first_time_experience_title", {
> +              "clientShortname": __("clientShortname2")

nit: same

@@ +178,5 @@
> +            })}
> +          </header>
> +          <Button additionalClass="fte-button"
> +                  onClick={this.handleButtonClick}
> +                  caption={__("first_time_experience_button_label")} />

nit: same

@@ +851,5 @@
>      RoomEntry: RoomEntry,
>      RoomList: RoomList,
>      SettingsDropdown: SettingsDropdown,
> +    ToSView: ToSView,
> +    GettingStartedView: GettingStartedView,

This exports list used to be ordered alphabetically... can you fix that?

Comment 19

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

Attachment: Patch v1.2

The change to reference the follow-up bug was a good idea. I've updated the patch to use that URL now.

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Comment on attachment 8523597 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8523597 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopAPI.jsm
> @@ +459,5 @@
> > +     *
> > +     * Any errors thrown by the Mozilla pref API are logged to the console
> > +     * and cause false to be returned.
> > +     */
> > +    setLoopBoolPref: {
> 
> I already dislike the fact that we don't have a `MozLoopService.getPref()`
> function that uses `getPrefType`, but we can have that here:
> 
> `setPref: function(prefName, value, [prefType]){}` with an optional preftype
> for when you know that the pref isn't set in firefox.js (rare)
> 
> How does that sound?

Discussed over IRC. We'll handle this in a follow-up that will also update the callers to getLoopCharPref, as doing it in this bug will balloon the patch.

> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +1375,5 @@
> >        log.error("Error opening FxA settings", ex);
> >      }
> >    }),
> >  
> > +  openGettingStartedTour: Task.async(function() {
> 
> why Task.async when you're not passing in a generator?

switchToTabHavingURI is O(N), so this allows the click handler to return before this completes. It is effectively the same as using Services.tm.mainThread.dispatch but it is yield-able (debatable if useful, since nothing is yielding on it ATM). The function above uses this also.

Comment 20

[Maire Reavy [:mreavy]]

Jared -- I believe you are unblocked at this point;  let me know if that changes. Thanks for filing Bug 1099462!

Comment 21

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

Attachment: Patch v1.2 (rebased and styling fixed)

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8524160 [details] [diff] [review]
Patch v1.2 (rebased and styling fixed)

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

Thanks!

::: browser/components/loop/MozLoopAPI.jsm
@@ +612,5 @@
>          return MozLoopService.openFxASettings();
>        },
>      },
>  
> +    openGettingStartedTour: {

Please add a docblock comment here when you land this.

::: browser/components/loop/MozLoopService.jsm
@@ +1403,5 @@
>        log.error("Error opening FxA settings", ex);
>      }
>    }),
>  
> +  openGettingStartedTour: Task.async(function() {

same here.

Comment 23

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

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

Comment 24

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

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

Comment 25

[Mark Banner (:standard8)]

I thought the planned UX, according to http://people.mozilla.org/~dhenein/loop/rooms/loop-rooms-spec-4.jpg was that we'd show just the "Get Started" button, and not the rooms list - and then when the button was clicked, we'd show the UX tour and the rooms list with start conversation button.

Did that change, or was it just interpreted differently?

Comment 26

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #25)
> I thought the planned UX, according to
> http://people.mozilla.org/~dhenein/loop/rooms/loop-rooms-spec-4.jpg was that
> we'd show just the "Get Started" button, and not the rooms list - and then
> when the button was clicked, we'd show the UX tour and the rooms list with
> start conversation button.

And I just realised - the UX also shows that the Tabs are hidden until after Get Started has been pressed.

Comment 27

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

(In reply to Mark Banner (:standard8) from comment #25)
> Did that change, or was it just interpreted differently?

It was interpreted differently. Since this is only supposed to be shown for new users, I didn't expect that they would be signed in or have a populated conversation list.

I filed bug 1101754 to get this fixed.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8524160 [details] [diff] [review]
Patch v1.2 (rebased and styling fixed)

Approval Request Comment

requirement for Loop Rooms support for Aurora/35 (and also provides First Time Experience for the feature as a whole).

On m-c; testing with rooms server changes now live in production
FTE content being worked on, not done yet.  That lives outside the browser itself

[String/UUID change made/needed]: none

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c0bc86e8c000

Comment 30

[Paul Silaghi, QA [:pauly]]

(In reply to Romain Testard [:RT] from comment #1)
> * The links to ToS and privacy notice (bug 1084362)
>     - This will disappear after the first time someone joins a room created
> by the user
Also a restart is required after the above, so the links to disappear. Expected ?

Comment 31

[Romain Testard [:RT]]

Not expected no. I would not consider this a blocker to release though.

Comment 32

[Paul Silaghi, QA [:pauly]]

(In reply to Romain Testard [:RT] from comment #1)
> To be clear, there are 3 components which are displayed on FTU:
> * "Get started" button opening the tour in a new tab (bug 1083466)
> * The links to ToS and privacy notice (bug 1084362)
> * The Telefonica logo (bug 1074720)
Verified fixed FF 35b5, 36.0a2 (2014-12-18)

(In reply to Romain Testard [:RT] from comment #31)
> Not expected no. I would not consider this a blocker to release though.
Filed bug 1113640