-
Notifications
You must be signed in to change notification settings - Fork 609
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
STRF-3382 display facebook like button when enabled #1530
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
cc8d5d0
to
357ff96
Compare
@@ -230,6 +230,7 @@ <h2 class="productView-brand"{{#if schema}} itemprop="brand" itemscope itemtype= | |||
{{/if}} | |||
</div> | |||
{{> components/common/share}} | |||
{{#if settings.facebook_like_button}}{{{settings.facebook_like_button}}}{{/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.
Should this go in the common/share
component? Do we only want it to show on this page, or anywhere we have the share buttons?
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.
Facebook like button isn't the same as the other share buttons it's an iframe and not an icon for one. I think it's good to keep them separate also the other share icons are linked to addthisservices but facebook like is not. It also seems to be specifically for the product page.
{{else}} | ||
<amp-social-share class="amp-social-share" data-vars-product-link="{{url}}" data-vars-product-id="{{id}}" data-vars-product-name="{{name}}" data-vars-social-type="{{service}}" type="{{service}}"></amp-social-share> | ||
{{/if}} | ||
<amp-social-share class="amp-social-share" data-vars-product-link="{{url}}" data-vars-product-id="{{id}}" data-vars-product-name="{{name}}" data-vars-social-type="{{service}}" type="{{service}}"></amp-social-share> |
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.
It looks like you are blending this change with #1526. Can you separate them?
357ff96
to
563152b
Compare
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 should put the markup for the facebook like button into the theme to allow for better customization.
470816f
to
d2e7008
Compare
e0ea153
to
d658954
Compare
allowTransparency="true"> | ||
</iframe> | ||
{{/if}} | ||
{{/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.
EOL is missing.
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.
fixed.
@@ -230,6 +230,7 @@ <h2 class="productView-brand"{{#if schema}} itemprop="brand" itemscope itemtype= | |||
{{/if}} | |||
</div> | |||
{{> components/common/share}} | |||
{{> components/common/facebook-like-button}} |
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.
Is there a reason why we can't pull this component inside {{> components/common/share}}
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.
I thought it would be better to separate it out since everything else is a svg instead of an iframe and they are about sharing whereas this is about liking but no real reason. Also, the majority of share.html is ShowAddThisServices but facebook like isn't part of that.
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.
@bookernath do you have an opinion on this one ? If not this PR is good to go.
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.
I like the idea of putting this new component inside the share component.
If I were a theme developer that wanted to move my share icons around on the page, I'd probably expect Facebook to come along for the ride, so I think that would be more convenient.
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.
Made the requested changes @bookernath
d658954
to
ec5bc8b
Compare
ec5bc8b
to
86d26e7
Compare
86d26e7
to
ebc677a
Compare
What?
Display like button when it is enabled through control panel. This pull request is to fix a bug where the facebook like button would not display when enabled.
Tickets / Documentation
Add links to any relevant tickets and documentation.
Screenshots (if appropriate)
Attach images or add image links here.