Closed Bug 1074932 Opened 10 years ago Closed 10 years ago

Desktop client user can access product tour from gears menu

Categories

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

defect
Points:
3

Tracking

(firefox34 wontfix, firefox35 verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox34 --- wontfix
firefox35 --- verified
firefox36 --- verified
backlog Fx35+

People

(Reporter: RT, Assigned: jaws)

References

Details

(Whiteboard: [FTE])

User Story

As a FF browser user, I'd like to access the product tour even after I started using the product so that I can get reminded of what the features are. 

* Link to product tour available through the gear menu
* Link opens tour in a new tab

Attachments

(1 file)

      No description provided.
Blocks: 1062640
User Story: (updated)
Are we planning to ship UI Tour in 34? If so, I think this should block uplift.
No, in Fx35, marking as a Fx35 item.
Target Milestone: --- → mozilla35
Whiteboard: [rooms]
I don't see how this is related to rooms. Can we remove the whiteboard and blocking bug?
Blocks: 1044994
backlog: --- → Fx35?
Flags: needinfo?(rtestard)
We will likely want the URL to be slightly different (e.g. an additional query parameter) than the one launched from the "Get Started" button.
See Also: → 1083466
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #3)
> I don't see how this is related to rooms. Can we remove the whiteboard and
> blocking bug?
This is in scope for the Fx35 release, I thought the point of the [rooms] tag is to identify the bugs released with the initial rooms release in Fx35.
Am I getting this wrong Shell?
Flags: needinfo?(rtestard) → needinfo?(sescalante)
(In reply to Romain Testard [:RT] from comment #5)
> This is in scope for the Fx35 release, I thought the point of the [rooms]
> tag is to identify the bugs released with the initial rooms release in Fx35.

OK, I wasn't aware of that and it's non-obvious. We already have "blocking-loop: Fx35", the target milestone, and status-firefox35: affected which more clearly say "Firefox 35" so I wish we wouldn't invent new ways to mark bugs.
backlog: Fx35? → Fx35+
Flags: needinfo?(sescalante)
Target Milestone: mozilla35 → ---
To clarify the blocking-loop Fx35+ flag determines the targeted release. "[rooms]" in the whiteboard is specifically for bugs that are needed for the Rooms feature.
Whiteboard: [rooms]
Priority: -- → P1
Whiteboard: [FTE]
Hey Marco, can you add this to the current iteration?
Assignee: nobody → jaws
Iteration: --- → 36.3
Points: --- → 3
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Status: NEW → ASSIGNED
Added to IT 36.3
Flags: needinfo?(mmucci)
Attached patch PatchSplinter Review
Cory, what kind of extra URL parameter do you want added to the patch to track that it came from the Gear menu? I've appended ?source=settingsMenu to the URL when it comes from the settings/gear menu. Is that enough for you? Do you have a specification/requirement for the names?

Michael, do you have an icon for this menu item? Right now it is the only menu item in the settings menu without an icon. The other ones are Sign in/Sign out and My Account (http://screencast.com/t/e6FANNf2MiNT)
Flags: needinfo?(mmaslaney)
Flags: needinfo?(cprice)
Attachment #8524846 - Flags: review?(MattN+bmo)
Hi Jared,

Question: is it an option to have multiple parameters appended to the URL?

e.g. ?utm_campaign=abc&utm_source=def&utm_medium=ghi

Thanks
Flags: needinfo?(cprice) → needinfo?(jaws)
Comment on attachment 8524846 [details] [diff] [review]
Patch

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

r=me with fixes

::: browser/components/loop/MozLoopService.jsm
@@ +1412,2 @@
>     */
> +  openGettingStartedTour: Task.async(function(aSrc) {

Denote that it's optional by providing the default value in the argument list e.g.:
aSrc = null

@@ +1414,4 @@
>      try {
>        let url = Services.prefs.getCharPref("loop.gettingStarted.url");
> +      if (aSrc) {
> +        url += (url.contains("?") ? "&" : "?") + "source=" + aSrc;

It would be good to just use a DOM URL object here instead of implementing it ourselves.
url.searchParams.set("source", aSrc) is easier to read IMO.

::: browser/components/loop/content/js/panel.jsx
@@ +287,5 @@
>      },
>  
> +    openGettingStartedTour: function() {
> +      navigator.mozLoop.openGettingStartedTour("settingsMenu");
> +      this.hideDropdownMenu();

Shouldn't hideDropdownMenu be instead called upon the click of every SettingsDropdownEntry? I mean that it can probably be done inside the SettingsDropdownEntry implementation.

@@ +314,5 @@
>                                     icon="account"
>                                     displayed={this._isSignedIn()} />
> +            <SettingsDropdownEntry label={mozL10n.get("tour_label")}
> +                                   onClick={this.openGettingStartedTour}
> +                                   displayed={true} />

I believe @displayed isn't necessary
Attachment #8524846 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8524846 [details] [diff] [review]
Patch

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

r=me with fixes

::: browser/components/loop/MozLoopService.jsm
@@ +1412,2 @@
>     */
> +  openGettingStartedTour: Task.async(function(aSrc) {

Denote that it's optional by providing the default value in the argument list e.g.:
aSrc = null

@@ +1414,4 @@
>      try {
>        let url = Services.prefs.getCharPref("loop.gettingStarted.url");
> +      if (aSrc) {
> +        url += (url.contains("?") ? "&" : "?") + "source=" + aSrc;

It would be good to just use a DOM URL object here instead of implementing it ourselves.
url.searchParams.set("source", aSrc) is easier to read IMO.

::: browser/components/loop/content/js/panel.jsx
@@ +287,5 @@
>      },
>  
> +    openGettingStartedTour: function() {
> +      navigator.mozLoop.openGettingStartedTour("settingsMenu");
> +      this.hideDropdownMenu();

Shouldn't hideDropdownMenu be instead called upon the click of every SettingsDropdownEntry? I mean that it can probably be done inside the SettingsDropdownEntry implementation.

@@ +314,5 @@
>                                     icon="account"
>                                     displayed={this._isSignedIn()} />
> +            <SettingsDropdownEntry label={mozL10n.get("tour_label")}
> +                                   onClick={this.openGettingStartedTour}
> +                                   displayed={true} />

I believe @displayed isn't necessary
(In reply to Cory Price [:ckprice] from comment #11)
> Question: is it an option to have multiple parameters appended to the URL?
> 
> e.g. ?utm_campaign=abc&utm_source=def&utm_medium=ghi

Yeah, we can put multiple parameters on it. What are you looking for? Should this be handled in a follow-up bug (in other words, will this need more discussions)?
Flags: needinfo?(jaws) → needinfo?(cprice)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Yeah, we can put multiple parameters on it. What are you looking for? Should
> this be handled in a follow-up bug (in other words, will this need more
> discussions)?

Yeah, if this bug is ready, please feel free to close with placeholder parameters.

I've opened bug 1101286 to track the URL parameter. We should have both URL questions answered in the next couple of days.
Flags: needinfo?(cprice)
(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22; away Nov. 14-19) from comment #13)
> > +    openGettingStartedTour: function() {
> > +      navigator.mozLoop.openGettingStartedTour("settingsMenu");
> > +      this.hideDropdownMenu();
> 
> Shouldn't hideDropdownMenu be instead called upon the click of every
> SettingsDropdownEntry? I mean that it can probably be done inside the
> SettingsDropdownEntry implementation.

This was here in error, as I had accidentally started working on this in the availability dropdown menu code. I didn't notice this remnant since it was called at the end of the event handler and all of the behavior matched the other menuitems. With more digging I filed bug 1101705, since none of the menuitems close the panel.

Thanks for the review, landed as https://hg.mozilla.org/integration/fx-team/rev/5f82b789efa3
Whiteboard: [FTE] → [FTE][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5f82b789efa3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [FTE][fixed-in-fx-team] → [FTE]
Target Milestone: --- → mozilla36
Comment on attachment 8524846 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Hard to re-access First Time Experience tour
[Describe test coverage new/current, TBPL]: on m-c
[String/UUID change made/needed]: none
Attachment #8524846 - Flags: approval-mozilla-aurora?
Attachment #8524846 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-moztrap?
In FF 35b5, the 'Tour' gear menu button has no icon.
In FF 36, 37 it has the icon.
Thoughts ?
Flags: needinfo?(jaws)
(In reply to Paul Silaghi, QA [:pauly] from comment #20)
> In FF 35b5, the 'Tour' gear menu button has no icon.
> In FF 36, 37 it has the icon.
> Thoughts ?

That's bug 1106991, which I'm about to land on beta in a few minutes.
Flags: needinfo?(jaws)
Verified fixed FF 35b5, 36.0a2(2014-12-19) Win 7.
Note that the 'Help' button from the gear menu opens the same http://mzl.la/1p3JqIy link.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mmaslaney)
You need to log in before you can comment on or make changes to this bug.