Closed Bug 1017314 Opened 6 years ago Closed 6 years ago

Make FTU tutorial configurable

Categories

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

x86_64
Linux
defect
Not set

Tracking

(feature-b2g:2.0, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
borjasalguero
: review+
borjasalguero
: feedback+
Details | Review
Bug 939174, Bug 990035 require the tutorial to be populated with different steps and content based on the device size/layout, and whether this is an upgrade or not. See Bug 939174 for discussion and context. 

I'm proposing with get existing functionality working and tests passing in this bug, then the particulars of the upgrade paths and vertical-homescreen updates on top of it, to minimize foot-stepping and conflicts.
Blocking the other tutorial changes on this
Assignee: nobody → sfoster
Blocks: 990036, 990035, 939174
WIP: https://github.com/sfoster/gaia/compare/configurable-tutorial
Functionality works as-is, I'm working on getting tests to pass. Can I get a thumbs up for this direction Franciso? Then I'll push forward on gettting this review-ready.
Flags: needinfo?(francisco)
Note: the preload() call in the evenhandling in ui.js is not great obviously. However, preload returns a promise and is idempotent - it only makes the XHR request the first time and just returns the existing promise after that. So we could safely call that earlier as well to get a headstart on loading the config. 
I've not included the changes in your and gmarty's branches to handle the update scenario, but this call moves again when we do that so I'm not so worried about it.
Feedback+ here from me, I'm tagging Borja as needinfo since he has in mind a different idea for the menu refactoring, not having to specify the image in each steps.

For me works fine, since we could potentially reuse resources.

Borja, opinion here?
Flags: needinfo?(francisco) → needinfo?(borja.bugzilla)
Attached file Comments in PR
See my note in the PR about the unit tests, any insight appreciated. 

I noticed that navigation.js has a steps structure like { "1" : { ... }, "2": ... }. I'm happy to move to using step numbers as keys instead of the array if prefered (the array just gets us a easy step count.)

Also, see previous comments about the async config loading in preload. We could have init call preload if it doesn't already have a config, but I'm a bit allergic to methods that may or may not be async. 

Also, Tutorial is experiencing some stylistic schizophenia, with some state on Tutorial/this and some via closure. I'm happy to tidy this up in a follow up bug.
Attachment #8431377 - Flags: review?(borja.bugzilla)
Attachment #8431377 - Flags: review?(angelfqc.18)
Here's how this plays out when I update the tutorial for the vertical homescreen - which uses video to illustrate gestures: https://github.com/sfoster/gaia/compare/configurable-tutorial...bug-990035-video-tutorial (WIP in bug 990035)
(In reply to Sam Foster [:sfoster] from comment #5)
> Created attachment 8431377 [details] [review]
> Comments in PR
> 
> See my note in the PR about the unit tests, any insight appreciated. 
> 
> I noticed that navigation.js has a steps structure like { "1" : { ... },
> "2": ... }. I'm happy to move to using step numbers as keys instead of the
> array if prefered (the array just gets us a easy step count.)
This may change in the near future, as we have some bugs dealing with this (927807, 916826)
feature-b2g: --- → 2.0
(In reply to Fernando Campo (:fcampo) from comment #7)
> (In reply to Sam Foster [:sfoster] from comment #5)
> > Created attachment 8431377 [details] [review]
> > Comments in PR
> > 
> > See my note in the PR about the unit tests, any insight appreciated. 
> > 
> > I noticed that navigation.js has a steps structure like { "1" : { ... },
> > "2": ... }. I'm happy to move to using step numbers as keys instead of the
> > array if prefered (the array just gets us a easy step count.)
> This may change in the near future, as we have some bugs dealing with this
> (927807, 916826)

I don't see any recent movement in those bugs and they don't have patches. We need this for 2.0 so we can't block on any other refactoring here. Can we get the review started please?
(In reply to Gregor Wagner [:gwagner] from comment #8)
> (In reply to Fernando Campo (:fcampo) from comment #7)
> > (In reply to Sam Foster [:sfoster] from comment #5)
> > > Created attachment 8431377 [details] [review]
> > > Comments in PR
> > > 
> > > See my note in the PR about the unit tests, any insight appreciated. 
> > > 
> > > I noticed that navigation.js has a steps structure like { "1" : { ... },
> > > "2": ... }. I'm happy to move to using step numbers as keys instead of the
> > > array if prefered (the array just gets us a easy step count.)
> > This may change in the near future, as we have some bugs dealing with this
> > (927807, 916826)
> 
> I don't see any recent movement in those bugs and they don't have patches.
> We need this for 2.0 so we can't block on any other refactoring here. Can we
> get the review started please?
I'm not trying to block anything whatsoever, but the opposite, meaning that we don't even need to use number-keys instead of arrays, as that will probably change in the future.
(In reply to Fernando Campo (:fcampo) from comment #9)
> (In reply to Gregor Wagner [:gwagner] from comment #8)
> > (In reply to Fernando Campo (:fcampo) from comment #7)
> > > (In reply to Sam Foster [:sfoster] from comment #5)
> > > > Created attachment 8431377 [details] [review]
> > > > Comments in PR
> > > > 
> > > > See my note in the PR about the unit tests, any insight appreciated. 
> > > > 
> > > > I noticed that navigation.js has a steps structure like { "1" : { ... },
> > > > "2": ... }. I'm happy to move to using step numbers as keys instead of the
> > > > array if prefered (the array just gets us a easy step count.)
> > > This may change in the near future, as we have some bugs dealing with this
> > > (927807, 916826)
> > 
> > I don't see any recent movement in those bugs and they don't have patches.
> > We need this for 2.0 so we can't block on any other refactoring here. Can we
> > get the review started please?
> I'm not trying to block anything whatsoever, but the opposite, meaning that
> we don't even need to use number-keys instead of arrays, as that will
> probably change in the future.

Perfect. Thanks :)
Comment on attachment 8431377 [details] [review]
Comments in PR

Hi Sam! Great job with this patch! I've added minor comments to Github, please take a look and ensure that Travis is happy with the code! (Marionette one is not related with this code).

Let me know when the PR is ready and I'll review it again, we are quite close to land this. Thanks!
Attachment #8431377 - Flags: review?(borja.bugzilla) → feedback+
Flags: needinfo?(borja.bugzilla)
I added comments on the PR regarding the preload() call in UIManager.init(). 

I also just reset my master and ran the #FTU unit tests. They "pass" because most are being skipped with a "MocksHelper is not defined" exception, so 0 tests get run. I'm not sure why mocha or travis consider this passing. So I think I'll go ahead and expand on the setup.js and misc. test fixes in this patch.
(In reply to Sam Foster [:sfoster] from comment #12)
> I added comments on the PR regarding the preload() call in UIManager.init(). 
> 
> I also just reset my master and ran the #FTU unit tests. They "pass" because
> most are being skipped with a "MocksHelper is not defined" exception, so 0
> tests get run. I'm not sure why mocha or travis consider this passing. So I
> think I'll go ahead and expand on the setup.js and misc. test fixes in this
> patch.
That will be fixed on bug 1017893, hopefully to be merged soon.
Ah excellent, that's pretty much line for line with my changes. I'll be able to pull that stuff out of my PR as soon as it lands.
Depends on: 1017893
I've updated my PR. This overlaps with bug 1017893 and depending on which one lands first, may need to rebase. My tests fix-up is pretty much identical except I used loadBodyHTML in the navigation test which allowed me to delete mock_navigation_html.js as well. 

On reflection on the preload/init async/sync thing, I went with spelling preload as 'loadConfig' to be more explicit, and added a ensureConfig (sugar) method which init calls before proceeding with most of the initialization. 

Caught a few other mistakes and address PR nits. Just waiting for travis :)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S3 (6june)
Comment on attachment 8431377 [details] [review]
Comments in PR

I must have missed somethign in that last pull request and got a (tangentially related) unit test failure. Can I get review on this meantime so I can potentially land this weekend if/when travis goes green? 

It seems Borja was happy with this minus the comments he left on the PR - which are now addressed.
Attachment #8431377 - Flags: review?(angelfqc.18) → review?(fernando.campo)
Comment on attachment 8431377 [details] [review]
Comments in PR

If Borja already started reviewing this, let's not gonna change reviewers mid-work (unless he doesn't have the time, so I can jump in)
Attachment #8431377 - Flags: review?(fernando.campo) → review?(borja.bugzilla)
Hi Sam! I've relaunched the Travis gaia-ui-test because it was related with the 'wallpaper' patch, so let's wait Travis to be green! I'll re-test it in my device when Travis is ready and I'll review it again. Great job!
Comment on attachment 8431377 [details] [review]
Comments in PR

Hi Sam,
While testing I've realized that as we need to load a file for the Tutorial, there is a 'flickering' effect when moving from the old panel to the Tutorial one.

After playing a bit with the code, the solution would be to shield our code against this, ensuring that all resources are loaded before changing the CSS property to show/hide the panels. I've created a small patch [1] which will shield your code against this scenario, and the transition between the panels is smooth. Please add this to your code (feel free to use your code style! This is only a proposal you can use), and ensure that all resources are ready before hiding the previous panel. Let me know when this is ready and I'll review and retest this again! 

[1] https://github.com/borjasalguero/gaia/commit/80e41e0bfbf3129ff7b2247ab315b36fe16a5514
Attachment #8431377 - Flags: review?(borja.bugzilla)
Thanks for that, it looks good to me. I got that patch merged and fixed up the unit tests and re-pushed: https://travis-ci.org/mozilla-b2g/gaia/builds/26583795
Comment on attachment 8431377 [details] [review]
Comments in PR

PR updated, I merged in the patch from comment 19 and fixed the one test it broke.
Attachment #8431377 - Flags: review?(borja.bugzilla)
Comment on attachment 8431377 [details] [review]
Comments in PR

Please wait until Travis is green and let's merge this patch. Great job! Thanks! Gracias! ;)
Attachment #8431377 - Flags: review?(borja.bugzilla) → review+
Travis is almost green, but keeps aborting the marionette_js job. As FTU tutorial has no marionette js tests to regress I'm going to go ahead and merge this. gaia-try(1) is green but for a perma-orange contacts integration test and a dialer unit test timeout. 


1. https://tbpl.mozilla.org/?tree=Gaia-Try&rev=437095cfc0a4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
Blocks: 1023829
No longer blocks: 1023829
You need to log in before you can comment on or make changes to this bug.