Js improvements #296

cshold · 2014-12-18T16:44:35Z

Fixes #281 and fixes #273

Adds fastclick
Move customer js to the bottom of the page
Move comment hash JS helpers to Timber's shop.js
Move login/recover password JS to Timber's shop.js

Demo shop for new updates - http://steve-dev.myshopify.com/

… file

cshold · 2014-12-18T16:46:44Z

assets/shop.js.liquid

+  }
+};
+
+timber.resetPasswordSuccess = function() {


Thoughts on this approach? This is called from login.liquid before timber.init() so the cached selectors won't exist yet. This checks if they exist (which we assume they don't) and gets them ready.

can it be moved to inside of timber.init ?

I don't think so. A liquid variable from within the submitted form on the page triggers this.

can it be re-written so that liquid sets a flag to true that init then checks? It's preferable to move all JS to init especially if in this scenario causes an awkward second call to cacheSelectors()

Take another look at the JS file and this line. I set a global variable inline, and then that is checked in shop.js.

Set timber.global object inline (article page for comments and reset password flag)

This can't be a liquid variable instead since they aren't transferred between templates/assets

No duplicate calls to cacheSelectors

Extend timber.globalVars into timber.global, which may already be set

Any up/downsides to extending this way? Are the names obvious enough, with a bit of documentation?

Edit: ignore the above comments. Going with another direction.

stevebosworth · 2014-12-18T19:41:34Z

templates/customers/login.liquid

@@ -91,7 +97,7 @@
        <p>
          <input type="submit" class="btn" value="{{ 'customer.recover_password.submit' | t }}">
        </p>
-        <a href="#" onclick="hideRecoverPasswordForm();return false;">{{ 'customer.recover_password.cancel' | t }}</a>
+        <a href="#" id="HideRecoverPasswordLink">{{ 'customer.recover_password.cancel' | t }}</a>


should we change these to buttons if they're not actually navigating anywhere?

mpiotrowicz · 2014-12-18T21:12:27Z

templates/article.liquid

              </script>
            {% endif %}

            {% if form.posted_successfully? %}
              <script>
-                window.location.hash = '#Comments';
+                $(function() {


one approach is to do something like var theme = window.theme || {}; themeVars.page.newHash = 'Comments' and inside of timber's init:

if (theme.page.newHash) { timber.updateHash(theme.page.newHash); }

If we get to the point where this is the last piece of inline JS I think it's worth it if that means we can move all scripts to the bottom of the page

mpiotrowicz · 2014-12-23T15:31:27Z

assets/shop.js.liquid

+    $hideRecoverPasswordLink: $('#HideRecoverPasswordLink'),
+    $recoverPasswordForm: $('#RecoverPasswordForm'),
+    $customerLoginForm: $('#CustomerLoginForm'),
+    $passwordResetSuccess: $('#ResetSuccess')


can you align these on the right? Realized it's not in our styleguide but it really helps readability (will add to the guide)

$recoverPasswordLink : $('#RecoverPassword'), $hideRecoverPasswordLink: $('#HideRecoverPasswordLink'), $recoverPasswordForm : $('#RecoverPasswordForm'), $customerLoginForm : $('#CustomerLoginForm'), $passwordResetSuccess : $('#ResetSuccess')

Doesn't that run into a similar problem of having all vars on one line during commits, in that adding a longer variable name would have to re-indent all lines? Worth it though eh?

++ I really like the readability

well, potentially, but you'd likely account for that in initially creating the object to begin with, but I think the readability tradeoffs are worth it

stevebosworth · 2014-12-23T15:45:12Z

I like this approach. I would add a comment where you define timber.global in shop.js to indicate that this is where we're keeping template specific values generated by liquid before timber inits.

Also, just for debates sake, maybe timber.window instead of timber.global? Might make it a little clearer what the purpose of the global object is?

mpiotrowicz · 2014-12-23T15:49:47Z

templates/article.liquid

+                window.timber = window.timber || {};
+                timber.global = {
+                  newHash: 'Comments'
+                };


i'm not sure that these need to be saved onto the Timber namespace as it's a little unexpected that you'd potentially be defining timber in this little page-script, and I'm not sure what nesting under global gives you, especially since this is very page specific (eg something like the cart is global, available on every page).

I'd consider re-naming this to be timber.pageSpecific or something similar, or creating a new global called templateVars and having timber check for that? None of these solutions are particularly great tbh. There's no way for you to set a liquid var here that's evaluated at the bottom of the page where timber can be called directly?

…ments

cshold · 2014-12-23T21:14:31Z

@stevebosworth can you give this a review please. There was a lot of back and forth with so don't worry too much about the comments, just the final code.

Things we've done

Moved all functions into shop.js
Set liquid variables in templates that will trigger Timber's JS after it is initialized
- This removes nearly all inline JS (addresses.liquid is an outlier here)
- Keeps all template-specific JS together (in theme.liquid
Cleaned up a lot of unneeded comments, mostly across customer templates

New demo page is at https://carson-dev-shop.myshopify.com/

stevebosworth · 2014-12-23T22:36:37Z

templates/article.liquid

-              <script>
-                window.location.hash = '#Comments';
-              </script>
+              {% assign newHash = 'Comments' %}
            {% endif %}


stevebosworth · 2014-12-24T02:52:21Z

Looking good! 👍

cshold · 2015-01-05T19:51:03Z

Any final comments on this PR @mpiotrowicz @stevebosworth?

mpiotrowicz · 2015-01-05T19:55:44Z

layout/theme.liquid

+    If you store has customer accounts disabled, you can remove the following JS file
+  {% endcomment %}
+  {% if template contains 'customers' %}
+    {{ 'shopify_common.js' | shopify_asset_url | script_tag }}


is this ok at the bottom of the page? Just want to confirm

Yup, everything works nicely still. Here's the file if you're curious.

mpiotrowicz · 2015-01-05T19:56:29Z

layout/theme.liquid

@@ -163,41 +152,62 @@

  {% include 'footer' %}

-  {% comment %}
-    Helper scripts that you can extend as needed.
-  {% endcomment %}
  {{ 'shop.js' | asset_url | script_tag }}


is this no longer the case with shop.js? (ie. the comments above)

It is, though we're going to continue nudging people toward a separate theme JS file rather than directly in shop.js. I'm going to rename it to timber.js so it is more obvious as well.

stevebosworth · 2015-01-05T23:06:06Z

@mpiotrowicz's comments aside, LGTM 👍

Js improvements across the board

cshold added 3 commits December 18, 2014 10:33

Added fastclick. Moved modernizer to footer

6c6b7cc

Moved customer JS to footer

a0c1690

Moved inline comment and login/password JS helpers to external Timber…

2fc44d8

… file

cshold reviewed Dec 18, 2014
View reviewed changes

cshold added 3 commits December 18, 2014 12:21

Updated IDs on login/password forms

8c61f8c

Update comment hash on doc ready. Update tab order

1bae734

Proper hidden labels for other account templates

0ed169c

stevebosworth reviewed Dec 18, 2014
View reviewed changes

Further account page js updates. Leaving jquery/modernizr in head

5df8fce

mpiotrowicz reviewed Dec 18, 2014
View reviewed changes

cshold added 2 commits December 23, 2014 09:39

Cleaned up inline js approach

e76a209

Removed console

28945dc

mpiotrowicz reviewed Dec 23, 2014
View reviewed changes

cshold added 3 commits December 23, 2014 11:23

Inline template liquid vars, triggers js after timber.init

60d7060

Conditional doc.read calls. Removed/edited some customer template com…

c5cb473

…ments

updated address tempalte comments

f286764

stevebosworth reviewed Dec 23, 2014
View reviewed changes

Remove erroneous options parameter in shop.js

99a2219

mpiotrowicz reviewed Jan 5, 2015
View reviewed changes

Renamed timber's js file

992a3e9

Updated modernizr version

ad77d5e

cshold added a commit that referenced this pull request Jan 7, 2015

Merge pull request #296 from Shopify/js-improvements

3f70d3b

Js improvements across the board

cshold merged commit 3f70d3b into master Jan 7, 2015

cshold deleted the js-improvements branch January 7, 2015 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Js improvements #296

Js improvements #296

cshold commented Dec 18, 2014

cshold Dec 18, 2014

mpiotrowicz Dec 18, 2014

cshold Dec 18, 2014

mpiotrowicz Dec 18, 2014

cshold Dec 23, 2014

stevebosworth Dec 18, 2014

mpiotrowicz Dec 18, 2014

mpiotrowicz Dec 23, 2014

cshold Dec 23, 2014

stevebosworth Dec 23, 2014

mpiotrowicz Dec 23, 2014

stevebosworth commented Dec 23, 2014

mpiotrowicz Dec 23, 2014

mpiotrowicz Dec 23, 2014

cshold commented Dec 23, 2014

stevebosworth Dec 23, 2014

stevebosworth commented Dec 24, 2014

cshold commented Jan 5, 2015

mpiotrowicz Jan 5, 2015

cshold Jan 5, 2015

mpiotrowicz Jan 5, 2015

cshold Jan 5, 2015

stevebosworth commented Jan 5, 2015

Js improvements #296

Js improvements #296

Conversation

cshold commented Dec 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevebosworth commented Dec 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshold commented Dec 23, 2014

Choose a reason for hiding this comment

stevebosworth commented Dec 24, 2014

cshold commented Jan 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevebosworth commented Jan 5, 2015