Add cart cookies disabled message #101
Conversation
There was a problem hiding this comment.
Thanks for getting this started, @ruairiphackett
I think slate.cart.checkCookies can be useful, but I'll ask that you change the implementation.
Slate is meant to stay as un-opinionated as possible. Part of this means only including classes when necessary; and when we do we keep them limited to helper classes like visually-hidden which is meant for accessibility.
In this PR (https://github.com/Shopify/slate/pull/98/files) you can see that I am pulling out all the classes in favour of data-attributes. I'm not saying themes built with Slate can't use classes, but I don't want the use of classes to get in the way of someone customizing it to suit their needs. It's what will let the product JS use this.variants = new slate.Variants(options); without worrying about any classes from the theme dev/designer.
The other thing about the PR you've proposed: If slate.cart.checkCookies() is going to be a helper function, it will need to work with little concern about the particular markup of the theme.
slate/variants.js -> No theme dep., but used by product.js
sections/product.js -> Theme dep., but asks slate/variants.js for information
Now if you look at slate/rte.js, you'll see a fair number of classes, like .rte. In this case it's somewhat unavoidable - we need to be able to distinguish RTE embeds from any other page embed.
How this applies to my requested changes:
- Remove the BEM cart classes from the markup and CSS files
- Change
slate/cart.jsto usereturnso it's a function another script could use (like in the case of product.js), or - Introduce a more generic class similar to Slate's
.no-js:not(html)that will be able to show/hide things depending whether cookies are supported.
|
@NathanPJF Cheers for the feedback 👍 Working on this messed up my fragile slate setup for |
|
@NathanPJF I went with the generic classes Do you think I should keep |
There was a problem hiding this comment.
Do you think I should keep
cart.jsunderslate/or should it betemplates/?
A cookie check is good for themes that want to enable an ajax cart - so I wouldn't limit it to a template. For example: displaying the cart contents in the drawer from the homepage and seeing that message.
The cookie check does feel like it would be more of a "utility" or "helper" function - but seeing as utils.js is only methods to help in dealing with arrays and objects, cart.js is making the most sense for the time being.
| @@ -40,6 +40,22 @@ | |||
| } | |||
| } | |||
|
|
|||
| // Only show when cookies are not supported | |||
| .no-cookies:not(html) { | |||
There was a problem hiding this comment.
I feel that a cookies class is too generic; however, we made a change in class names for our most recent theme along these lines already. To express support for JS in a class that made more sense we switched to: supports-js and supports-no-js.
Following the same logic:
// Only show when browser cookies are not supported
.supports-no-cookies:not(html) {
display: none;
html.supports-no-cookies & {
display: block;
}
}
// Only show when browser cookies are supported
.supports-cookies {
html.supports-no-cookies & {
display: none;
}
}| </div> | ||
|
|
||
| {% comment %} | ||
| Cart no cookies state |
There was a problem hiding this comment.
Please put a small blurb in here that explains browser cookies are required to build a cart
src/templates/cart.liquid
Outdated
| <p>{{ 'cart.general.continue_browsing_html' | t }}</p> | ||
|
|
||
| {% comment %} | ||
| Cart Empty State |
There was a problem hiding this comment.
Cart empty state
^ letter-casing
src/scripts/theme.js
Outdated
| @@ -31,4 +32,9 @@ $(document).ready(function() { | |||
| // Wrap videos in div to force responsive layout. | |||
| slate.rte.wrapTable(); | |||
| slate.rte.iframeReset(); | |||
|
|
|||
| // Add a message to the cart if cookies are disabled. | |||
There was a problem hiding this comment.
This function won't directly display the message, that's taken care of by CSS. Suggested reword:
Apply a specific class to the html element for browser support of cookies.
src/scripts/slate/cart.js
Outdated
| slate.cart = { | ||
|
|
||
| /** | ||
| * Check if cookies are enabled in the browser |
There was a problem hiding this comment.
Please put a small blurb in here that explains browser cookies are required to build a cart
|
@NathanPJF I changed the class names and added the explanations you requested, thanks for the quick review 👍 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fix for https://github.com/Shopify/shopify-themes/issues/3355
I was a little unsure of where to locate the js for the cart or whether or not to make cart into a section.