Update to sections and variant js #24

stevebosworth · 2016-10-17T21:51:12Z

@Shopify/themes-fed

Updates Slate to use variants.js instead of option_selection.js
Adds sections.js
Switched product template to a section

cshold · 2016-10-18T13:06:44Z

src/scripts/sections/product.js

+    var $container = this.$container = $(container);
+
+    this.settings = {
+      enableHistoryState: $container.data('enable-history-state'),


default to true if data attribute not set

cshold · 2016-10-18T13:07:37Z

src/scripts/sections/product.js

+      this.variants = new slate.Variants(options);
+
+      this.$container.on('variantChange' + this.settings.eventNamespace, this.updateVariantSelection.bind(this));
+


whitespace 👮

cshold · 2016-10-18T13:08:18Z

src/scripts/sections/product.js

+     */
+    updateVariantSelection: function(evt) {
+      var variant = evt.variant;
+      console.log(variant);


cshold · 2016-10-18T13:09:04Z

src/scripts/sections/product.js

+     * @param {boolean} enabeled - Decides whether cart is enabled or disabled
+     * @param {string} text - Updates the text notification content of the cart
+     */
+    updateAddToCartState: function(enabeled, text) {


spelling: "enabled"

cshold · 2016-10-18T13:10:33Z

src/scripts/sections/product.js

+      console.log(variant);
+      // Update cart button and text status
+      if (!variant) {
+        this.updateAddToCartState(false, theme.strings.unavailable);


sending true/false here is confusing because you have to go find the function to see what it is. I'd recommend this.updateAddToCartState(theme.strings.unavailable, 'disabled'); — if the second param is not set it can stay as the default enabled/true

cshold · 2016-10-18T13:11:06Z

src/scripts/sections/product.js

+     *
+     * @param {object} product - Product object
+     */
+    simplifyVariantLabels: function(product) {


This can be done in Liquid now

cshold · 2016-10-18T13:11:42Z

src/scripts/sections/product.js

+     * @param {string} src - Image src URL
+     */
+    updateProductImage: function(src) {
+      var sizedImgUrl = slate.Image.getSizedImageUrl(src, this.settings.imageSize);


Set the main image's size as part of this.settings so that if we want to preload images we already know what size to load

cshold · 2016-10-18T13:13:32Z

src/scripts/slate/variants.js

+    var currentOptions = this._getCurrentOptions();
+    this.currentVariant = this._getVariantFromOptions(currentOptions);
+
+    $(this.selectors.singleOptionSelector).on('change', this._onSelectChange.bind(this));


It's a bit strange that this required selector is merely a part of this.settings rather than it's own option. Thoughts?

ya, i was thinking about that. might make more sense to pass in the two required selectors explicitly. I don't see the number of selectors ballooning down the road

It's just the one required selector, no?

originalSelectorId and singleOptionSelector

cshold · 2016-10-18T13:14:10Z

src/sections/product.liquid

@@ -0,0 +1,100 @@
+<div id="ProductSection-{{ section.id }}" data-section-id="{{ section.id }}" data-section-type="product" itemscope itemtype="http://schema.org/Product">


id not needed for anything

cshold · 2016-10-18T13:15:14Z

src/scripts/sections/product.js

+      eventNamespace: '.product'
+    };
+
+    this.selectors = {


Should probably add sectionId to these selectors, knowing we're likely to have a "Featured product" home page section eventually

I was thinking about that, I think we'd want to build that out as part of slate or just leave it as is... let's discuss during our planning session

cshold · 2016-10-18T18:32:56Z

src/scripts/slate/variants.js

@@ -20,12 +20,14 @@ slate.Variants = (function() {
    this.$container = options.$container;
    this.product = options.product;
    this.selectors = options.selectors;


remove this.selectors with the latest update

cshold · 2016-10-18T18:35:26Z

src/scripts/slate/variants.js

@@ -20,12 +20,14 @@ slate.Variants = (function() {
    this.$container = options.$container;
    this.product = options.product;
    this.selectors = options.selectors;
+    this.singleOptionSelector = options.singleOptionSelector;
+    this.originalSelectorId = options.originalSelectorId;
    this.settings = options.settings;


Can remove this.settings in favour of this.enableHistoryState since that's the only one we actually use

t-kelly · 2016-10-18T20:04:25Z

src/scripts/sections/product.js

+      eventNamespace: '.product'
+    };
+
+    this.selectors = {


This has been bugging me - why are we placing the selectors inside the constructor? It bloats it so much 😢

In this case it's not needed, but it will be if we add sectionId to the selectors like in Debut to support multiple product sections on the page.

You really shouldn't need to add sectionId to every selector - just the container, no? Reduces the complexity of both our JS and Liquid.

switch them to classes and that would work yea 👍

t-kelly · 2016-10-18T20:04:50Z

src/scripts/sections/product.js

+
+    // Stop parsing if we don't have the product json script tag when loading
+    // section in the Theme Editor
+    if (!$('#ProductJson').html()) {


Why aren't we using a selector here

t-kelly · 2016-10-18T20:07:54Z

src/scripts/sections/product.js

+    initVariants: function() {
+      var options = {
+        $container: this.$container,
+        settings: this.settings,


Is settings a standard object? i.e. Do we always know what's inside of it? Kind of vague passing an object like this.

true, I updated to use named options instead of passing in objects from the product

t-kelly · 2016-10-18T20:08:42Z

src/scripts/sections/product.js

+      };
+
+      // eslint-disable-next-line no-new
+      this.variants = new slate.Variants(options);


Is options used anywhere else? Why not inline it and drop the variable declaration?

I honestly have no preference between these two approaches and can think of arguments supporting both. 👍 for inline object, 🎉 for separate var?

Slight preference to options var, but not much. This is how I always did it in WordPress land with args passed to helpers

t-kelly · 2016-10-18T20:10:52Z

src/scripts/sections/product.js

+      // eslint-disable-next-line no-new
+      this.variants = new slate.Variants(options);
+
+      this.$container.on('variantChange' + this.settings.eventNamespace, this.updateAddToCartState.bind(this));


Realized we don't have to be as careful with event handlers as I originally thought. Only with event handlers that are tied to document. Event handlers are cleaned up by the garbage collector when the object it is bound too is removed from the DOM. Source

I think we no longer need the namespacing (in most cases).

oh that's interesting! 🤔 I do like being able to call off(this.settings.eventNamespace) for TE events though. I wonder if the TE always rebuilds the DOM or not though... we might not need to remove any events that aren't attached to document in the onUnload event...

t-kelly · 2016-10-18T20:14:28Z

src/scripts/slate/variants.js

+    this.$container = options.$container;
+    this.product = options.product;
+    this.selectors = options.selectors;
+    this.singleOptionSelector = options.singleOptionSelector;


I still feel like we should have defaults set for these options so that you don't need to include them when initializing unless they are different from the default values. How often do you use a different selector across themes?

I'd like to avoid hardcoding things like that into variant.js since you may want different selectors depending on how you build the DOM. It think these values should be set in the themes product.js instead. One example would be radio buttons vs non-radio button. Also if you decide not to use BEM it's one more place to track down a class name.

t-kelly · 2016-10-18T20:15:20Z

src/scripts/slate/variants.js

+    this.settings = options.settings;
+
+    var currentOptions = this._getCurrentOptions();
+    this.currentVariant = this._getVariantFromOptions(currentOptions);


We never got to combining these two functions? _getCurrentOptions() and _getVariantFromOptions()?

I don't think it's a good idea to combine them now that _getCurrentOptions is a little more complicated, but i think you're right about them being tied together so I moved _getCurrentOptions() callsite inside _getVariantFromOptions

t-kelly · 2016-10-18T20:16:25Z

src/scripts/slate/variants.js

+        }
+      });
+
+      options = _.filter(options);


What is this doing? It has no truth test?

it's removing unchecked values (if you're using a radio button), looking through lodash again, _.compact is a better choice. The values returned in the array are a string or false so this works too but is maybe a bit confusing.

t-kelly · 2016-10-18T20:18:08Z

src/scripts/slate/variants.js

+     * @return {k}         [description]
+     */
+    _updateHistoryState: function(variant) {
+      if (!history.pushState || !variant) {


Should probably also be checking for replaceState()

one doesn't really exist without the other but I can switch this to replaceState since that's what we're using

cshold · 2016-10-19T20:16:02Z

error Unexpected template string expression no-template-curly-in-string

In currency.js. Warnings can be fixed in another PR. 👍

bertiful · 2016-10-19T20:45:46Z

There's a few other warnings as well.

🚢

* suggested updates for CLI help content * updates based on PR feedback * adding back missing aliases

lock · 2018-10-26T23:49:06Z

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.

stevebosworth added 3 commits October 17, 2016 17:02

WIP updating slate JS

09be9e2

updates product to variant js

2a13b38

updated comments

caf69ac

stevebosworth self-assigned this Oct 17, 2016

removed option selection comment

c53757c

cshold suggested changes Oct 18, 2016

View reviewed changes

stevebosworth added 2 commits October 18, 2016 11:48

updates to use variant events instead of single function

eca87e1

use selector strings instead of object

53e719d

cshold reviewed Oct 18, 2016

View reviewed changes

t-kelly reviewed Oct 18, 2016

View reviewed changes

stevebosworth force-pushed the update-variant-js branch 2 times, most recently from 06b783a to 704ccd7 Compare October 19, 2016 18:30

cshold approved these changes Oct 19, 2016

View reviewed changes

stevebosworth force-pushed the update-variant-js branch from 704ccd7 to 9817522 Compare October 19, 2016 20:48

fixes for PR

5252669

stevebosworth force-pushed the update-variant-js branch from 9817522 to 5252669 Compare October 19, 2016 20:59

stevebosworth merged commit 94fcf71 into master Oct 20, 2016

stevebosworth deleted the update-variant-js branch October 20, 2016 14:32

stevebosworth had a problem deploying to staging October 26, 2016 17:59 Failure

stevebosworth had a problem deploying to staging October 26, 2016 18:25 Failure

stevebosworth had a problem deploying to staging October 26, 2016 18:46 Failure

stevebosworth had a problem deploying to staging October 26, 2016 18:58 Failure

stevebosworth had a problem deploying to staging October 26, 2016 19:14 Failure

stevebosworth deployed to staging October 26, 2016 19:17 Active

stevebosworth had a problem deploying to production November 3, 2016 22:00 Failure

stevebosworth had a problem deploying to production November 7, 2016 18:48 Failure

stevebosworth had a problem deploying to production November 7, 2016 20:06 Failure

stevebosworth had a problem deploying to production November 7, 2016 20:14 Failure

stevebosworth had a problem deploying to production November 7, 2016 20:22 Failure

stevebosworth had a problem deploying to production November 7, 2016 20:58 Failure

stevebosworth deployed to production November 7, 2016 21:23 Active

t-kelly pushed a commit that referenced this pull request Jul 31, 2017

suggested updates for CLI help content (#24)

33f7ce6

* suggested updates for CLI help content * updates based on PR feedback * adding back missing aliases

t-kelly pushed a commit that referenced this pull request Jul 31, 2017

suggested updates for CLI help content (#24)

45d18cd

* suggested updates for CLI help content * updates based on PR feedback * adding back missing aliases

t-kelly pushed a commit that referenced this pull request Jul 31, 2017

suggested updates for CLI help content (#24)

5122e89

* suggested updates for CLI help content * updates based on PR feedback * adding back missing aliases

lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018

		this.variants = new slate.Variants(options);

		this.$container.on('variantChange' + this.settings.eventNamespace, this.updateVariantSelection.bind(this));

		@@ -0,0 +1,100 @@
		<div id="ProductSection-{{ section.id }}" data-section-id="{{ section.id }}" data-section-type="product" itemscope itemtype="http://schema.org/Product">

Update to sections and variant js #24

Update to sections and variant js #24

Conversation

stevebosworth commented Oct 17, 2016

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshold commented Oct 19, 2016

bertiful commented Oct 19, 2016

lock bot commented Oct 26, 2018