-
Notifications
You must be signed in to change notification settings - Fork 606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stencil AMP ⚡ #946
Stencil AMP ⚡ #946
Conversation
I'm getting the following error when I try to compile this locally:
Are there additional compile steps needed? |
@nickdengler you need to update |
@mjschock would love your eyes on this |
assets/js/app.js
Outdated
if (PageTypeFn) { | ||
const pageType = new PageTypeFn(context); | ||
if (loadGlobal) { | ||
globalClass = new Global; |
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 we invoke it like new Global()
here? it just looks strange.
} | ||
|
||
load() { | ||
async.series([ |
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.
nice - i like that it's here rather than in app.js
{{#each settings.add_this.buttons}} | ||
{{#if service '!==' 'google'}} | ||
{{#if service '!==' 'print'}} | ||
{{#if service '!==' 'facebook'}} |
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.
wondering if we could use #all here with subexpressions:
{{#all (if service '!==' 'google') (if service '!==' print') (if service '!==' 'facebook')}}...
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.
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?
@@ -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}}: |
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.
🍹 just looks odd to me with the empty line. maybe one of the following instead?
{{display_name}}:
{{#if required}}
<small>{{lang 'common.required'}}</small>
{{/if}}
{{display_name}}<span>:</span>
{{#if required}}
<small>{{lang 'common.required'}}</small>
{{/if}}
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.
we are using :
all over the templates, not using the entity :
value="{{id}}" | ||
{{#if selected}}checked{{/if}} | ||
{{#if ../required}}required{{/if}}> | ||
|
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.
remove empty line (like in set-radio.html)
templates/layout/amp.html
Outdated
{{#block "amp-style"}} {{/block}} | ||
|
||
|
||
{{{head.rsslinks}}} |
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.
nit; do we need these empty lines
overall, LGTM 👍 a few minor nitpickings though |
2cd94f2
to
6e48f10
Compare
LGTM 💚 Awesome work all around! |
Squeezing commits and rebasing before merging |
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. |
What?
Stencil AMP support for product and category pages
Tickets / Documentation
Screenshots (if appropriate)
@bigcommerce/stencil-team