-
Notifications
You must be signed in to change notification settings - Fork 487
Conversation
@pauldpritchard Fixed the cart alignment. As for a load indicator, there are |
} | ||
}); | ||
}; | ||
|
||
formOverride = function () { | ||
$formContainer.submit(function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change to use .on('submit', function(evt) to stay current with the styleguide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure. This was pre-stylguide so thanks for catching.
Would be great to get another set of eyes from @Shopify/fed on this. For context, this PR drastically simplifies the ajax cart plugin in Timber, along with adding slide out drawers (from marketing assets) and a mobile navigation. More comments at the top. |
@carolineschnapp are you able to take a look at this too? |
@cshold thanks for explaining the marketing assets stuff. Took another look and looks good to me. I have some questions about the JS but nothing that needs fixing necessarily. 👍 |
See my last commit @carolineschnapp @stevebosworth Instead of that function being used, you can now bind with:
I hadn't thought of the straight copy/paste of a snippet, but with that in mind this is definitely the better way to go. What do you think of the implementation? |
I think app developers needed the custom event. Good-citizen apps that perform actions on the cart “page” — and there are many of those — are encouraged to use the script_tag API. This means when the app is installed, an externally-hosted JavaScript file is loaded, and when same app is removed, that JS file is no longer loaded. Based on the need to run JavaScript independently from a theme's files — without modifying a theme's files, the custom event thing gets a +1. Otherwise you need a lot of code to behave responsibly in your own JavaScript — you cannot undermine what other people may be doing: if (typeof afterCartLoad === 'undefined') {
var afterCartLoad = function(data) {
/* Do something */
}
}
else {
var original_afterCartLoad = window.afterCartLoad;
var afterCartLoad = function(data) {
original_afterCartLoad(data);
/* Do something */
};
} There are also my own “selfish” reasons: a dozen tutorials in our docs that will be easily revamped to work with all of our newer themes. I am scared I may have swayed you in a wrong direction, though @cshold , I am no JavaScript developer, so I am curious what others may think of this. CC @Shopify/design-gurus |
.... and that fixed it. I am able to implement multiples currencies the same way I always have, except I add this code to my code, and voilà: jQuery('body').on('ajaxCart.afterCartLoad', function(evt, cart) {
Currency.convertAll(shopCurrency, jQuery('[name=currencies]').val());
}); |
Really happy with this! 💱 🍰 ❕ 🎆 |
@@ -0,0 +1,627 @@ | |||
/*============================================================================ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so is ajaxify now ajax-cart.js? Why the rename? (out of curiosity)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean cut away from the old version. If bugs pop up around the ajax cart, we'll instantly know if it's v1 or v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/cshold/Timber/commit/f9c046f83b1572ec4160815c66a5fa43cdddfda3 for the changes between ajax-cart.js and ajaxify.js.
General note on the size of this PR - the description was super valuable to see what's been updated, but this both touches a lot of files and a lot of functionality was rolled into this single PR. Can these be split up in the future? Will really help with reviews |
@include at-query ($min, $large) { | ||
padding: $gutter 0; | ||
} | ||
.grid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a pretty big change for .grid
, if it affects all grids inside of .site-header
, what about creating a variant class for the grid in this case? Reading the markup, if there aren't any other clues that the grid changes, I'd be surprised by this behavior. Why is it necessary within the site-header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just removes the media query around the styles so that elements always use the table layout. Agreed though, will use variation class.
Ajax cart api
Mobile menu and new ajax cart drawer
Demo - https://carson-dev-shop.myshopify.com/
Issues addressed
Adding a mobile menu fixes #292
Ajax cart callback fixes #244
Code details
Drawers
timber.drawersInit
sets up multiple drawers (in this case menu and cart)Drawers
module from Shopify's Marketing Assets (private repo, sorry public folk)evt
detectedtimber.RightDrawer.open()
after a product is added to the pageAjax Cart
ajaxify.scss.liquid
and moved simplified styles intotimber.scss.liquid
. JS remains in a separate filesnippets/ajax-cart-template.liquid
instead of using.load()
fortemplates/cart.liquid
. This gives us more flexibility in the different layouts given their use.Other changes
<button>
reset stylesCart callback
A few issues were tracked because the ajax cart's callback ran before the DOM was actually updated. This PR fixes that and lets you define a callback function when initializing the cart plugin the same way the Shopify
option_selection.js
does on the product page.Updated docs PR - #352
@stevebosworth @mpiotrowicz @pauldpritchard