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

VERIFIED FIXED in Firefox 35

Status

P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: MattN, Assigned: jaws)

Tracking

unspecified
mozilla36
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-moztrap ?
qe-verify +

Firefox Tracking Flags

(firefox34 wontfix, firefox35 verified, firefox36 verified)

Details

(Whiteboard: [FTE])

Attachments

(1 attachment, 3 obsolete attachments)

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.
Flags: qe-verify+
Flags: firefox-backlog+

Comment 1

4 years ago
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

Updated

4 years ago
Whiteboard: [rooms]

Updated

4 years ago
Whiteboard: [rooms] → [FTE]

Updated

4 years ago
backlog: Fx35? → Fx35+
Priority: -- → P1
Duplicate of this bug: 1086509
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 3
Have we decided on a URL yet?
Flags: needinfo?(rtestard)
Flags: needinfo?(mreavy)

Comment 5

4 years ago
hi Cory,  do we know the URL of the tab to open/switchTo for the tour yet?
Flags: needinfo?(cprice)
We have not decided on a URL for this. What is our deadline to do so?

Comment 7

4 years ago
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....
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!
Flags: needinfo?(cprice) → needinfo?(sescalante)
(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?
(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.
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 ;).
(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
Flags: needinfo?(sescalante)
Created attachment 8523306 [details] [diff] [review]
Patch
Attachment #8523306 - Flags: review?(mdeboer)
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 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.
Created attachment 8523597 [details] [diff] [review]
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 :)
Attachment #8523306 - Attachment is obsolete: true
Attachment #8523306 - Flags: review?(mdeboer)
Attachment #8523597 - Flags: review?(mdeboer)
Attachment #8523597 - Flags: review?(MattN+bmo)
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".
Attachment #8523597 - Flags: review-

Updated

4 years ago
Flags: needinfo?(rtestard)
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?
Attachment #8523597 - Flags: review?(mdeboer)
Created attachment 8523995 [details] [diff] [review]
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.
Attachment #8523597 - Attachment is obsolete: true
Attachment #8523995 - Flags: review?(mdeboer)
Attachment #8523597 - Flags: review?(MattN+bmo)
Jared -- I believe you are unblocked at this point;  let me know if that changes. Thanks for filing Bug 1099462!
Flags: needinfo?(mreavy)
Created attachment 8524160 [details] [diff] [review]
Patch v1.2 (rebased and styling fixed)
Attachment #8523995 - Attachment is obsolete: true
Attachment #8523995 - Flags: review?(mdeboer)
Attachment #8524160 - Flags: review?(mdeboer)
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.
Attachment #8524160 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/e15cb9b33827
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [FTE][fixed-in-fx-team] → [FTE]
Target Milestone: --- → mozilla36
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?
(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.
(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 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
Attachment #8524160 - Flags: approval-mozilla-aurora?
Attachment #8524160 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
status-firefox35: affected → fixed
status-firefox36: --- → fixed
Flags: in-moztrap?
(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 ?
Flags: needinfo?(rtestard)
Not expected no. I would not consider this a blocker to release though.
Flags: needinfo?(rtestard)
(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
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
status-firefox36: fixed → verified
You need to log in before you can comment on or make changes to this bug.