Start to hook up each of the screen modules. #13

shane-tomlinson · 2013-11-07T00:26:51Z

@borjasalguero, @sergi, @OlavHN - I have taken the code that I wrote during the Madrid work week and ported it here, attempting to follow naming conventions as much as possible. There is a lot of duplication in the modules - for instance set-password and enter-password are very similar. I did this intentionally to come up with approaches tailored to the screen and then create a generalization out of those approaches. I am not doing any lazy loading of the modules yet, but this should be possible.

I was able to simplify fxam_navigation.js a bit, and the ability to go forward and back through the states works well.

Items that still need completed:

To test, some values are hard coded:

To follow new user flow - enter newuser@newuser.com.
To follow existing user flow - enter an other email address.
The only acceptable password is password

* Add Utils.once which is a one time event handler.

borjasalguero · 2013-11-08T14:09:09Z

@shane-tomlinson Hi Shane! I'll try to take a look today to the code!!!! :)

borjasalguero · 2013-11-11T14:42:26Z

apps/system/fxa/js/fxam_error_overlay.js

+      DIALOG_SELECTOR = '#fxa-error-overlay',
+      TITLE_SELECTOR = '#fxa-error-title',
+      MESSAGE_SELECTOR = '#fxa-error-msg',
+      CLOSE_BUTTON_SELECTOR = '#fxa-error-ok';


In this case I would avoid the use of $. Instead you could use the following idea:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L38

With this approach you are caching DOM elements (you can execute this in the init method), and we would adapt your code to Gaia code style! :)

borjasalguero · 2013-11-11T14:45:42Z

apps/system/fxa/elements/fxa-signup-success.html

+        <a id="fxa-summary-email"></a>, your Firefox Account is ready to go!
+      </p>
+      <p data-l10n-id="fxa-signin-success-use-services">
+        Now you can use <span id="ff_account--success-email">cookies</span> to access services like Marketplace and Where’s My Fox.


In this case we should avoid the mix of hash & underscore. In this case this would be ffaccount-success-email to follow Gaia code guidelines.

borjasalguero · 2013-11-11T14:47:13Z

apps/system/fxa/fxa_module.html

@@ -6,6 +6,7 @@
    <link href="../shared/style/headers.css" rel="stylesheet" type="text/css">
    <link href="../shared/style/switches.css" rel="stylesheet" type="text/css">
    <link href="../shared/style/buttons.css" rel="stylesheet" type="text/css">
+    <link href="../shared/style/confirm.css" rel="stylesheet" type="text/css">


Why do we need confirm?

borjasalguero · 2013-11-11T14:47:55Z

apps/system/fxa/fxa_module.html

    <script src="shared/js/lazy_loader.js"></script>
    <script src="shared/js/l10n.js"></script>
    <script src="js/utils.js"></script>
    <script src="js/fxam_overlay.js"></script>
+    <script src="js/fxam_error_overlay.js"></script>


We should add defer, and a LazyLoading of the elements.

borjasalguero · 2013-11-11T14:50:32Z

apps/system/fxa/fxa_module.html

-
+
+    <!-- state modules -->
+    <script src="js/fxam_intro.js"></script>


Same here, and this would be one of the key points. As we don't know the entire set of steps from the beginning, we need to lazy load the steps. So imagine that we are on the fist step fxam_intro.js, when clicking next we would lazyload this code, only if needed (saving a lot of memory). We would need something like https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/startup.js#L100 for loading the module asap.

borjasalguero · 2013-11-11T14:51:19Z

apps/system/fxa/fxa_module.html

-
+
+    <!-- Error message overlay -->
+    <form role="dialog" data-type="confirm" id="fxa-error-overlay" class="fade-in">


Could be a simple alert?

I tried an alert and nothing was displayed. This dialog comes from the FTU code itself, so I thought I would keep it consistent. This is also why confirm.css is loaded.

borjasalguero · 2013-11-11T14:55:55Z

apps/system/fxa/js/fxam_enter_email.js

+ * Module checks the validity of an email address, and if valid,
+ * determine which screen to go to next.
+ */
+FxaModuleEnterEmail = (function() {


As all modules have the same structure, why dont we create a Module object, and FxaModuleEnterEmail will extend basic Module object. Wdyt? With this approach we would have a clear structure of a Module.

borjasalguero · 2013-11-11T14:57:08Z

apps/system/fxa/js/fxam_enter_email.js

+  }
+
+  function showInvalidEmail() {
+    // TODO - Hook up to i18n


Let's create l10n entities! :)
For loading them we use something lie var _ = navigator.mozL10n.get;, so asking for the label l10n-email would be _('l10n-email') :)

borjasalguero · 2013-11-11T14:57:51Z

apps/system/fxa/js/fxam_enter_email.js

+    // User can abort FTE without entering an email address.
+    if ( ! email) return done(FxaModuleStates.DONE);
+
+    console.log("entered email", email);


Remove logs! :)

borjasalguero · 2013-11-11T14:58:43Z

apps/system/fxa/js/fxam_enter_email.js

+    }, 500);
+  }
+
+  var Module = {


Probably this could be the structure of the Module object should have, and the structure that all the modules should inherit

borjasalguero · 2013-11-11T15:00:45Z

apps/system/fxa/js/fxam_error_overlay.js

@@ -0,0 +1,38 @@
+var FxaModuleErrorOverlay = (function() {


Could we change this code following same structure as https://github.com/borjasalguero/gaia/blob/fxa_proposal_complete/apps/system/fxa/js/fxam_overlay.js ?

borjasalguero · 2013-11-11T15:01:13Z

apps/system/fxa/js/fxam_error_overlay.js

+    show: function fxam_error_overlay_show(title, message) {
+      var overlayEl = $(DIALOG_SELECTOR);
+      var titleEl = $(TITLE_SELECTOR);
+      var messageEl = $(MESSAGE_SELECTOR);


This is a general thing, but caching element is needed in all JS classes.

borjasalguero · 2013-11-11T15:03:16Z

apps/system/fxa/js/fxam_navigation.js

@@ -9,42 +10,30 @@ var FxaModuleNavigation = {
    // Load view
    LazyLoader._js('view/view_' + flow + '.js', function loaded() {
      this.view = View;
-      this.maxSteps = Object.keys(View).length;
-      this.currentStep = this.view[0];
+      this.maxSteps = View.length;


If I'm not wrong, with steps approach we dont know actually the max number of steps, because if sign in & sign up differs in some way (imagine that UX adds a new step in the middle for sign up), this could not work... Maybe we should think how to deal with the navigation without using maxSteps... :(

borjasalguero · 2013-11-11T15:13:12Z

When testing, in the email step, I have the following:

E/GeckoConsole( 3471): [JavaScript Error: "TypeError: step is null" {file: "app://system.gaiamobile.org/fxa/js/fxam_ui.js" line: 39}]
E/GeckoConsole( 3471): Content JS LOG at app://system.gaiamobile.org/fxa/js/fxam_ui.js:38 in FxaModuleUI.loadStep: step null 2

:(. Furthermore when testing I can not use the keyboard in this step, and I dont know why (previously was working).

Regarding the new code, main things to be fixed are:

Lazy load resources. Now we are loading all steps with all the JS tied to that steps. We should load step by step.
Inheritance: Module object. All modules/steps are clearly instances of an object Module (actually your code is following the patter, we only have to use inheritance!), so we would need to create it, and every step will be an extension of that object.
Cache UI elements. Avoid the use of $ and cache elements during the init method.
Naming. Use Gaia conventions. Avoid mix of _ and - in the same class name.
l10n & linting. We should ensure that Travis is going to be happy with our linting, and l10n is something that we should add as we are adding patches, because at the end is going to be a pain to come back and review all pending l10n tags!

That's all from my side. If I can help you with this let me know and I will send you a PR to your branch! Thanks for your great job! :)

* Convert fxam_states to contain both the id of the element to load plus the name of the module to initialize. * The template element contains script elements which are scripts to lazy load.

Lazy load the FxA js

… modules.

* remove $ and declaring all the selectors at the top of the file * in init, call importElements.

Start to hook up each of the screen modules.

Shane Tomlinson added 8 commits Nov 6, 2013

Create modules for each of the FxA account handling screens.

7b301cc

Start to include the modules.

db7575a

Try to match up the navigation code with the expected module code.

74853f1

Add password reset, set password, success screens.

43b14ad

Returning users should go through the enter password flow.

847e9b8

make forward and back work in the navigaton.

dbcc58b

Adding an error dialog and related code.

56ec1af

* Add Utils.once which is a one time event handler.

Hook up password incorrect screens.

3908932

borjasalguero reviewed Nov 11, 2013
View changes

apps/system/fxa/js/fxam_enter_email.js

// User can abort FTE without entering an email address.

if ( ! email) return done(FxaModuleStates.DONE);

console.log("entered email", email);

This comment has been minimized.

Sign in to view

borjasalguero Nov 11, 2013
Owner

Remove logs! :)

borjasalguero reviewed Nov 11, 2013
View changes

apps/system/fxa/js/fxam_enter_email.js

}, 500);

}

var Module = {

This comment has been minimized.

Sign in to view

borjasalguero Nov 11, 2013
Owner

Probably this could be the structure of the Module object should have, and the structure that all the modules should inherit

borjasalguero reviewed Nov 11, 2013
View changes

OlavHN and others added 7 commits Nov 12, 2013

Lazy load panel specific javascript

6a13e6e

Lazy load the HTML & JS for each of the FxA modules.

b19698e

* Convert fxam_states to contain both the id of the element to load plus the name of the module to initialize. * The template element contains script elements which are scripts to lazy load.

Merge pull request #2 from shane-tomlinson/lazy-load-js

f87c94e

Lazy load the FxA js

remove logs

e9c532f

Create a prototype fxam_module that is used as the base for all other…

de5d4c7

… modules.

cache necessary elements from init as is gaia's style.

7700e6c

* remove $ and declaring all the selectors at the top of the file * in init, call importElements.

Start to hook up l10n

f37329f

Shane Tomlinson added 2 commits Nov 12, 2013

Update structure to follow fxam_overlay.js

3c7caf3

Start to hook up error screens for enter and set password.

913a896

shane-tomlinson pushed a commit that referenced this pull request Nov 12, 2013

Merge pull request #13 from shane-tomlinson/fxa_proposal_complete

967c877

Start to hook up each of the screen modules.

shane-tomlinson merged commit 967c877 into borjasalguero:fxa_proposal_complete Nov 12, 2013

borjasalguero / gaia

Start to hook up each of the screen modules. #13

Start to hook up each of the screen modules. #13

shane-tomlinson commented Nov 7, 2013

borjasalguero commented Nov 8, 2013

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

borjasalguero commented Nov 11, 2013



		<!-- state modules -->
		<script src="js/fxam_intro.js"></script>



		<!-- Error message overlay -->
		<form role="dialog" data-type="confirm" id="fxa-error-overlay" class="fade-in">

borjasalguero / gaia

Start to hook up each of the screen modules. #13

Start to hook up each of the screen modules. #13

Conversation

shane-tomlinson commented Nov 7, 2013

borjasalguero commented Nov 8, 2013

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

shane-tomlinson Nov 12, 2013 Author

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

shane-tomlinson Nov 12, 2013 Author

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

This comment has been minimized.

borjasalguero Nov 11, 2013 Owner

borjasalguero commented Nov 11, 2013

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

shane-tomlinson Nov 12, 2013
Author

borjasalguero Nov 11, 2013
Owner

shane-tomlinson Nov 12, 2013
Author

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner

borjasalguero Nov 11, 2013
Owner