v1.3.0 #80
v1.3.0 #80
Conversation
cshold
changed the title
| @@ -1,5 +1,39 @@ | |||
| $(function() { | |||
| window.shop = window.shop || {}; | |||
There was a problem hiding this comment.
Thoughts on this structure for Timber's JS file? Before it was simply a docready call, but I'd prefer to push toward some kind of structure.
There was a problem hiding this comment.
I like it, only thing is might want to create a more specific var name, TimberShop or something
There was a problem hiding this comment.
👍 - a few thoughts, though they're by no means deal breakers:
- You're creating a global
shopobject if one doesn't exist, but then you're definingshop(line 3) as a local. If this code was wrapped in an IIFE, you'd be creating two objects which could be potentially tricky to diagnose. One way around this is to initialize the object (window.shop = window.shop || {}) then add properties and functions to it below, likeshop.init = function(){...}(novarkeyword). Added benefit: no need to use commas to separate properties. Alternatively, you could add properties to the shop's prototype likeshop.prototype.init = function(){...}, but that may add unnecessary complexity - Personally, I like to reserve
elfor a single element like Backbone does. That may just be preference though - I appreciate the DOM caching - 'tis good practice, though there's a few other selectors throughout. Maybe stuff all of them into your DOM cache object (
elcurrently)?
All in all though, I feel this is much more extensible and readable :)
| @include transition(all 0.1s ease-out); | ||
|
|
||
| :not(body.ajaxify-touch) & { |
There was a problem hiding this comment.
this won't work with IE8, assuming that's ok for Timber?
There was a problem hiding this comment.
Yup, just sets the border as transparent to get ready for a CSS transition so it can be ignored for IE8.
| @@ -1,5 +1,36 @@ | |||
| $(function() { | |||
| window.timber = window.timber || {}; | |||
There was a problem hiding this comment.
Looks like I lost the line comment after my update, but appreciate the notes. Rich, I assume this is what you meant by splitting the methods up?
Cached elements are more aptly named. Will consider moving other selectors up there too, though I sometimes prefer keeping the relevant ones with their methods.. just preference there.
There was a problem hiding this comment.
Comments are still in the conversation view under "Show outdated diff", but yep, that's what I meant!
🐓💨
cshold
changed the title
| <li> | ||
| <img src="{{ type | payment_type_img_url }}" /> | ||
| </li> | ||
| {% if type == "diners_club" %}<li>c</li>{% endif %} |
There was a problem hiding this comment.
why do it this way? also why no case statement?
There was a problem hiding this comment.
Using the icon font allows for flexibility when defining colour, similar to the social icons. Forgot about using case in liquid, I'll switch to using that.
There was a problem hiding this comment.
On second thought, it makes more sense to define the icons in css with content to keep the generic characters out of the markup.
There was a problem hiding this comment.
if implementing an icon font, can this be done in a less intrusive way? If the font fails there's no fallback and it's not accessible. Something like the following would be more robust
<span class="icon-fallback-text"> <span class="icon icon-visa" aria-hidden="true"></span> <span class="text">VISA</span> </span>
More info here under "Critical Icons with Text Fallback" - http://filamentgroup.com/lab/bulletproof_icon_fonts.html
There was a problem hiding this comment.
beat me to it! But yeah check out the article above for a really solid implementation
JavaScript
shop.js.liquidfile structureLiquid
Theme Settings
CSS