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

chore: update cookie consent banner script #334

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

nkuehn
Copy link
Collaborator

@nkuehn nkuehn commented Mar 31, 2020

We received a new cookie consent banner ID for the docs kit, here's my try to implement it correctly.

The raw script copy/paste we received form Marketing is:

<!-- OneTrust Cookies Consent Notice start -->
<script type="text/javascript" src="https://cdn.cookielaw.org/consent/b104027d-4d10-4b75-9675-9ffef11562a8/OtAutoBlock.js"></script>
<script type="text/javascript" src="https://cdn.cookielaw.org/scripttemplates/otSDKStub.js" type="text/javascript" charset="UTF-8" data-domain-script="b104027d-4d10-4b75-9675-9ffef11562a8"></script>
<script type="text/javascript">
  function OptanonWrapper() {}
</script>
<!-- OneTrust Cookies Consent Notice end -->

@vercel
Copy link

vercel bot commented Mar 31, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/commercetools/commercetools-docs-kit/8m1udb2y2
✅ Preview: https://commercetools-docs-kit-git-nk-update-cookie-banner-script.commercetools.now.sh

@nkuehn nkuehn requested a review from emmenko March 31, 2020 20:51
@vercel vercel bot temporarily deployed to Preview April 1, 2020 16:06 Inactive
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

@nkuehn I pushed a commit to:

  1. render scripts as list elements (setPostBodyComponents accepts a list of react components), no need for rendering them together with a fragment
  2. render the script elements with the defer option

This attribute allows the elimination of parser-blocking JavaScript where the browser would have to load and evaluate scripts before continuing to parse. async has a similar effect in this case.

@nkuehn
Copy link
Collaborator Author

nkuehn commented Apr 1, 2020

@emmenko thanks for spotting that you can pass a list (which is obvious by the name of the function, but I just did not take enough car).

Concerning deferring the execution I am not very inclined to restart the debug cycle with that horrible script. I am pretty sure that I already tried deferring the execution and it did not work or caused weird things like two complete cookie consent instances laid over each other, rendering the page overlay as black.

Another reason why I'm hesitant is that we can't really test it as easily without actually running it on a commercetools.com domain in production.

@emmenko
Copy link
Member

emmenko commented Apr 2, 2020

There should be no issue with defer as far as I can tell. The MDN doc says:

This Boolean attribute is set to indicate to a browser that the script is meant to be executed after the document has been parsed, but before firing DOMContentLoaded.
Scripts with the defer attribute will prevent the DOMContentLoaded event from firing until the script has loaded and finished evaluating.

The purpose of the attribute is just to eliminate the blocking of the parse phase of the browser. However, the script will still be loaded and executed before the page is officially loaded.

Anyway, if you still prefer to keep it out I can change that.

@vercel vercel bot temporarily deployed to Preview April 2, 2020 19:52 Inactive
@nkuehn
Copy link
Collaborator Author

nkuehn commented Apr 2, 2020

@emmenko sorry, clicked "update from master" which is not a rebase.

Let's try defer, by spec it should work - we just need to be very fast in rolling a complete kit release cycle when we find out it's blocking the complete custom apps docs after go-live on a commercetools.com domain.

@emmenko
Copy link
Member

emmenko commented Apr 2, 2020

I can try this in the custom apps docs first

@nkuehn nkuehn merged commit 2b869e2 into master Apr 2, 2020
@nkuehn nkuehn deleted the nk-update-cookie-banner-script branch April 2, 2020 20:09
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.

2 participants