Skip to content
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

CHECKOUT-4118 Remove remote_api_scripts from the templates #1505

Merged
merged 1 commit into from
Jun 6, 2019

Conversation

icatalina
Copy link
Contributor

What?

This should avoid double inclusion of some of the analytics code. (eg. facebook pixel)

Tickets / Documentation

https://jira.bigcommerce.com/browse/CHECKOUT-4118

Screenshots (if appropriate)

Before:
image

After:
image

@bigcommerce/storefront-team @lord2800 @megdesko

@bigbot
Copy link

bigbot commented May 22, 2019

Autotagging @bigcommerce/storefront-team @davidchin

This should avoid double inclusion of some of the analytics code. (eg. facebook pixel)
@lord2800
Copy link
Contributor

This looks right to me, but I'll let @bigcommerce/storefront-team make the final call here.

@aleachjr
Copy link

I suspect this might require regression testing anything included in remote api scripts. From what I recall this was required for Jirafe among other things.

@lord2800
Copy link
Contributor

It will definitely require regression testing, but this was not the right fix for Jirafe/etc. In fact, adding this caused more problems than it solved--removing it is the right fix, we just need to make sure all of our ducks are in a row before we do it.

@icatalina
Copy link
Contributor Author

can someone manage the regression testing in your side? I don't really know what's included using the remote_api_scripts tag. 🤷‍♂

@lord2800
Copy link
Contributor

@bc-sandeep you mind taking this on?

@mattolson
Copy link
Contributor

@icatalina You have done a great service with this PR. I think this is the correct thing to do. We'll organize some regression testing just to be sure.

Copy link
Contributor

@mattolson mattolson left a comment

Choose a reason for hiding this comment

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

The PR looks good, but let's do some basic regression testing on this by running the functional test suite against it. And then you'll need to loop back and come up with a fix for CP-4054 if that proves to be a regression.

@mattolson mattolson merged commit 96ce7d3 into master Jun 6, 2019
@mattolson mattolson deleted the CHECKOUT-4118 branch June 6, 2019 02:49
@bc-junbai
Copy link

I checked an integration store with Cornerstone version 3.5.0, and Problem #3 double firing addToCart event doesn't happen anymore.

LGTM 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants