-
Notifications
You must be signed in to change notification settings - Fork 19
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
Implemented the analytics consent banner for Playground #581
Implemented the analytics consent banner for Playground #581
Conversation
✅ Deploy Preview for thingweb-playground ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
The eslint error is a bit weird to see since that is coming from google analytics. Otherwise, the integration looks good. When I head over to google analytics, I see TD Playground as page title in the realtime analytics and I see Eclipse Thingweb in the report snapshot. So they do not show up at the same time somehow? Reading https://www.analyticsmania.com/post/subdomain-tracking-with-google-analytics-and-google-tag-manager/ means that we do not need multiple properties and with what I have observed, it is probably working to some extent. I have followed https://support.google.com/analytics/answer/10071811 and adding a condition and once this PR is merged, it should just work. |
I believe the eslint issue with the gtag function, is probably due to how Webpack handles the bundling,etc. Or it could also be that since the js bundle is not added until production, it cannot access the already instantiated gtag function in the HTML file. |
The banner is causing the tests to fail since playwright cannot click on the many links and buttons under the banner. I think the footer should be clicked or hidden for the tests. |
We can use snippet injection of netlify to add analytics: https://docs.netlify.com/site-deploys/post-processing/snippet-injection/ . But I leave it up to you @SergioCasCeb |
Yeah, sorry about that. I forgot to also push the changes to the tests. I should be done in just a bit |
Since the consent management, needs a really specific order to be executed, besides the fact that some of it depends on the local storage, I think it would be easier to just add it directly in the code. Besides Playground is a one page application, so it doesn't really complicate anything, besides also allowing for testing. |
After the recent changes, we now have 355 tests instead of 140 since the consent management tests are done for each. Can we optimize it by doing the consent management test once at the beginning of everything and then before each test, playwright just clicks deny and closes the banner? Otherwise the tests now pass |
Just out of curiosity, where did the 355 number come from, for me it is only 142 tests, as well as in the visual testing pipeline. I set it up exactly how you are explaining, before any test is done, the consent banner is checked for the default state, and the closed (so that is "one test"). Then there's an extra test to check that the update does occur when opening the banner and then changing the consent to allow (this being the 2nd test added) |
Ah my bad, I ran npx playwright test which ran for other browsers as well. Merging! |
Google Analytics, as well as the respective banner for consent management, have been implemented on TD Playground following Eclipse's requirements, while also maintaining the same overall design as the consent banner from the Thingweb website."