Stencil AMP ⚡ #946

mcampa · 2017-03-03T01:54:12Z

What?

Stencil AMP support for product and category pages

Tickets / Documentation

https://jira.bigcommerce.com/browse/STENCIL-2944

Screenshots (if appropriate)

@bigcommerce/stencil-team

nickdengler · 2017-03-06T19:53:25Z

I'm getting the following error when I try to compile this locally:

Validating theme...
/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/lib/stencil-bundle.js:252
            throw err;
            ^

Error: Your theme's config.json has errors:
config.meta.features[20] is not one of enum values: fully_responsive,mega_navigation,multi_tiered_sidebar_menu,masonry_design,frontpage_slideshow,quick_add_to_cart,switchable_product_view,product_comparison_table,complex_search_filtering,customizable_product_selector,cart_suggested_products,free_customer_support,free_theme_upgrades,high_res_product_images,product_filtering,advanced_quick_view,product_showcase,persistent_cart,one_page_check_out,product_videos
    at BundleValidator.validateThemeConfiguration (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/lib/bundle-validator.js:104:25)
    at /Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/node_modules/async/lib/async.js:718:13
    at iterate (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/node_modules/async/lib/async.js:262:13)
    at async.forEachOfSeries.async.eachOfSeries (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/node_modules/async/lib/async.js:281:9)
    at _parallel (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/node_modules/async/lib/async.js:717:9)
    at Object.async.series (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/node_modules/async/lib/async.js:739:9)
    at BundleValidator.validateTheme (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/lib/bundle-validator.js:56:11)
    at validateTheme (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/lib/stencil-bundle.js:250:15)
    at /Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/node_modules/async/lib/async.js:718:13
    at iterate (/Users/nick.dengler/.nvm/versions/node/v4.4.0/lib/node_modules/@bigcommerce/stencil-cli/node_modules/async/lib/async.js:262:13)

Are there additional compile steps needed?

junedkazi · 2017-03-06T19:55:05Z

@nickdengler you need to update stencil-cli

bookernath · 2017-03-07T07:33:47Z

@mjschock would love your eyes on this

mjschock · 2017-03-07T17:54:12Z

assets/js/app.js

-                if (PageTypeFn) {
-                    const pageType = new PageTypeFn(context);
+                if (loadGlobal) {
+                    globalClass = new Global;


can we invoke it like new Global() here? it just looks strange.

mjschock · 2017-03-07T18:00:07Z

assets/js/theme/page-manager.js

+    }
+
+    load() {
+        async.series([


nice - i like that it's here rather than in app.js

mjschock · 2017-03-07T18:06:10Z

templates/components/amp/common/share.html

+        {{#each settings.add_this.buttons}}
+          {{#if service '!==' 'google'}}
+            {{#if service '!==' 'print'}}
+              {{#if service '!==' 'facebook'}}


wondering if we could use #all here with subexpressions:

{{#all (if service '!==' 'google') (if service '!==' print') (if service '!==' 'facebook')}}...

Actually, maybe it would be better to put this logic into a helper and just return the computed html to the template. Easier to read, and likely less typing. Would that work?

mjschock · 2017-03-07T18:23:31Z

templates/components/amp/products/options/input-checkbox.html

@@ -0,0 +1,20 @@
+<div class="form-field" data-product-attribute="input-checkbox">
+    <label class="form-label form-label--alternate form-label--inlineSmall">
+        {{display_name}}:


🍹 just looks odd to me with the empty line. maybe one of the following instead?

{{display_name}}&colon; {{#if required}} {{lang 'common.required'}} {{/if}}

{{display_name}}&colon; {{#if required}} {{lang 'common.required'}} {{/if}}

we are using : all over the templates, not using the entity &colon;

mjschock · 2017-03-07T18:24:41Z

templates/components/amp/products/options/set-rectangle.html

+            value="{{id}}"
+            {{#if selected}}checked{{/if}}
+            {{#if ../required}}required{{/if}}>
+


remove empty line (like in set-radio.html)

mjschock · 2017-03-07T18:29:53Z

templates/layout/amp.html

+        {{#block "amp-style"}} {{/block}}
+
+
+        {{{head.rsslinks}}}


nit; do we need these empty lines

mjschock · 2017-03-07T18:32:13Z

overall, LGTM 👍 a few minor nitpickings though

nickdengler · 2017-03-09T22:14:37Z

LGTM 💚 Awesome work all around!

mcampa · 2017-03-09T22:31:01Z

Squeezing commits and rebasing before merging

junedkazi · 2017-03-09T22:44:20Z

We need a change log entry for this. Also since this is going to get merged first can we also add a new line at the end of last entry since if you don't the last release title wraps up.

mcampa changed the title ~~Stencil AMP~~ Stencil AMP ⚡ Mar 3, 2017

mcampa force-pushed the amp branch from bed724e to fd867e4 Compare March 3, 2017 01:56

mjschock reviewed Mar 7, 2017

View reviewed changes

templates/layout/amp.html Outdated

{{#block "amp-style"}} {{/block}}

{{{head.rsslinks}}}

Copy link

Contributor

mjschock Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; do we need these empty lines

mcampa force-pushed the amp branch 5 times, most recently from 2cd94f2 to 6e48f10 Compare March 9, 2017 02:30

mcampa force-pushed the amp branch from c361f8e to 4b84d04 Compare March 9, 2017 22:30

mcampa force-pushed the amp branch from 4b84d04 to 59a7ea4 Compare March 9, 2017 22:53

STENCIL-2944 Google AMP support for product and category pages

d3af229

mcampa force-pushed the amp branch from 59a7ea4 to d3af229 Compare March 9, 2017 22:56

mcampa merged commit 0d95be5 into bigcommerce:master Mar 9, 2017

mcampa deleted the amp branch March 9, 2017 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stencil AMP ⚡ #946

Stencil AMP ⚡ #946

mcampa commented Mar 3, 2017

nickdengler commented Mar 6, 2017

junedkazi commented Mar 6, 2017

bookernath commented Mar 7, 2017

mjschock Mar 7, 2017

mjschock Mar 7, 2017

mjschock Mar 7, 2017

tabayomi Mar 7, 2017

mjschock Mar 7, 2017

mcampa Mar 8, 2017

mjschock Mar 7, 2017

mjschock Mar 7, 2017

mjschock commented Mar 7, 2017

nickdengler commented Mar 9, 2017

mcampa commented Mar 9, 2017

junedkazi commented Mar 9, 2017

Stencil AMP ⚡ #946

Stencil AMP ⚡ #946

Conversation

mcampa commented Mar 3, 2017

What?

Tickets / Documentation

Screenshots (if appropriate)

nickdengler commented Mar 6, 2017

junedkazi commented Mar 6, 2017

bookernath commented Mar 7, 2017

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

mjschock commented Mar 7, 2017

nickdengler commented Mar 9, 2017

mcampa commented Mar 9, 2017

junedkazi commented Mar 9, 2017