Closed Bug 1409977 Opened 5 years ago Closed 5 years ago

define the property to mark Onboarding tour complete instantly

Categories

(Firefox :: Tours, enhancement, P5)

58 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox58 --- verified

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding][onboarding-modulize])

Attachments

(1 file, 1 obsolete file)

Instead of hardcode and check specific tour ids in source, we can also define a property in tourSet to denote this tour would be marked as complete instantly after user switched to this tour.
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify+
Summary: define the property to mark Onboarding tour autocomplete → define the property to mark Onboarding tour instantly complete
Summary: define the property to mark Onboarding tour instantly complete → define the property to mark Onboarding tour complete instantly
Component: New Tab Page → Tours
Attachment #8920444 - Attachment is obsolete: true
Assigning P3 since it seems to be in progress. Feel free to correct the priority and status flags.
Priority: -- → P3
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #5)
> Assigning P3 since it seems to be in progress. Feel free to correct the priority and status flags.
Thanks for the help.
Because this is an nice improvement, not really hurry in the current stage so set to P5.
Priority: P3 → P5
Whiteboard: [photon-onboarding]
Comment on attachment 8920443 [details]
Bug 1409977 - mark Onboarding tour will set as completed instantly via define a tour property;

https://reviewboard.mozilla.org/r/191416/#review198874

::: browser/extensions/onboarding/content/onboarding.js:1201
(Diff revision 3)
>  
>        let tab = this._window.document.createElement("span");
>        tab.id = tour.id;
>        tab.textContent = this._bundle.GetStringFromName(tour.tourNameId);
> -      tab.className = "onboarding-tour-item";
> +      tab.className = tour.instantComplete ?
> +        "onboarding-tour-item onboarding-instant-complete" : "onboarding-tour-item";

I was thinking using `data-onboarding-instant-complete` attribute since this is not about CSS style. How do you think?
Comment on attachment 8920443 [details]
Bug 1409977 - mark Onboarding tour will set as completed instantly via define a tour property;

https://reviewboard.mozilla.org/r/191416/#review198874

> I was thinking using `data-onboarding-instant-complete` attribute since this is not about CSS style. How do you think?

make sense, will do it
Comment on attachment 8920443 [details]
Bug 1409977 - mark Onboarding tour will set as completed instantly via define a tour property;

https://reviewboard.mozilla.org/r/191416/#review199394

r+ with some comments updates required

::: browser/extensions/onboarding/content/onboarding.js:33
(Diff revision 4)
>  /**
>   * Add any number of tours, key is the tourId, value should follow the format below
>   * "tourId": { // The short tour id which could be saved in pref
>   *   // The unique tour id
>   *   id: "onboarding-tour-addons",
> + *   // (optional) mark tour as complete instantly when user click the nav item

nit: "mark tour as complete instantly when user enters the tour" because we got the case enters tour without clicking nav item. The current code is correct but if followed the comment we should have moved the codes to another place.

::: browser/extensions/onboarding/content/onboarding.js:815
(Diff revision 4)
> -        tab.classList.remove("onboarding-active");
> -        tab.setAttribute("aria-selected", false);
> -      }
> -    }
>  
> -    switch (tourId) {
> +        // some tours should tagged completed instantly upon showing.

nit: "some tours should *be* tagged completed instantly upon showing." or "some tours should complete instantly upon showing." if you like shorter
Attachment #8920443 - Flags: review?(fliu) → review+
nit addressed, thanks for review
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/acc9f95343e7
mark Onboarding tour will set as completed instantly via define a tour property;r=Fischer
Comment on attachment 8920443 [details]
Bug 1409977 - mark Onboarding tour will set as completed instantly via define a tour property;

https://reviewboard.mozilla.org/r/191416/#review199394

> nit: "some tours should *be* tagged completed instantly upon showing." or "some tours should complete instantly upon showing." if you like shorter

This is not resolved but simply dropped. Please comment why not resovling or handle it. Why do you often either ignore issue or close issue without explanation... The important thing is making sure what we are doing.
> > nit: "some tours should *be* tagged completed instantly upon showing." or "some tours should complete instantly upon showing." if you like shorter
> 
> This is not resolved but simply dropped. Please comment why not resovling or
> handle it. Why do you often either ignore issue or close issue without
> explanation... The important thing is making sure what we are doing.

I've updated that part but not double checked the updated PR carefully...
Something is missing during my process, will do more carefully next time.
https://hg.mozilla.org/mozilla-central/rev/acc9f95343e7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
After revisit I found the mozreview board shows the change now... 

https://reviewboard.mozilla.org/r/191416/diff/5#index_header 
http://searchfox.org/mozilla-central/source/browser/extensions/onboarding/content/onboarding.js#816

So I did make the change (but I still missed to remove `d` after `complete`)
Whiteboard: [photon-onboarding] → [photon-onboarding][onboarding-modulize]
I wanted to to verify this bug on the latest nightly but I couldn't find any steps to follow to verify this bug.
Could you please provide some steps or scenarios?

Thanks.
Flags: needinfo?(gasolin)
Hi Hani,

make sure 

1. when you mouse click or auto show the [sync, performance, default-browser] tour
2. the check sign is shown on navigate items without click other button
Flags: needinfo?(gasolin)
I can confirm this issue is fixed, I verified using Fx 58.0b5, on Windows 10 x64, mac OS X 10.13.2 and Ubuntu 16.04 LTS.

Cheers!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.