1017314 - Make FTU tutorial configurable

Status: RESOLVED FIXED

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

Description

[Sam Foster [:sfoster] (he/him)]

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.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Blocking the other tutorial changes on this

Comment 2

[Sam Foster [:sfoster] (he/him)]

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.

Comment 3

[Sam Foster [:sfoster] (he/him)]

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.

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

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?

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: 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.

Comment 6

[Sam Foster [:sfoster] (he/him)]

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)

Comment 7

[Fernando Campo (:fcampo)]

(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)

Comment 8

[Gregor Wagner [:gwagner]]

(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?

Comment 9

[Fernando Campo (:fcampo)]

(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.

Comment 10

[Gregor Wagner [:gwagner]]

(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 11

[Borja Salguero [:borjasalguero]]

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!

Comment 12

[Sam Foster [:sfoster] (he/him)]

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.

Comment 13

[Fernando Campo (:fcampo)]

(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.

Comment 14

[Sam Foster [:sfoster] (he/him)]

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.

Comment 15

[Sam Foster [:sfoster] (he/him)]

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 :)

Comment 16

[Sam Foster [:sfoster] (he/him)]

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.

Comment 17

[Fernando Campo (:fcampo)]

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)

Comment 18

[Borja Salguero [:borjasalguero]]

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 19

[Borja Salguero [:borjasalguero]]

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

Comment 20

[Sam Foster [:sfoster] (he/him)]

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 21

[Sam Foster [:sfoster] (he/him)]

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.

Comment 22

[Borja Salguero [:borjasalguero]]

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! ;)

Comment 23

[Sam Foster [:sfoster] (he/him)]

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

Comment 24

[Sam Foster [:sfoster] (he/him)]

Landed: https://github.com/mozilla-b2g/gaia/commit/e43011c698274d4721c8466e4d7cc561d017b1fd

Comment 25

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.