-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add cookie banner #344
Add cookie banner #344
Conversation
Tests should be run before merging this PR. Some checks to consider:
Updated the PR description |
I think we have to merge to test it in staging environment right? |
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 found a little issue following this steps:
- The localStorage is empty
- The banner is displayed automatically and the user clicks on "Accept All"
- The user clicks on "Preferences" and the banner is displayed with the "Analytics" checkbox checked.
- The user unchecks the checkbox and clicks on "Accept All"
- The user clicks on "Preference" and the banner is displayed, but the checkbox is unchecked and it should be checked. The localStorage works correctly, but not the checkbox.
- Refreshing the page fixes the issue after step 5.
Edit:
I found that this is also happening at htts://safe.global. As it is just a UI issue happening on the first visit (fixed after refreshing once) and the localStorage works good, I'm fine leaving this as it is for now an not spending more time (unless is something super easy to fix).
Co-authored-by: Germán Martínez <[email protected]>
Latest commit makes two changes:
|
Context
This PR adds the cookie banner to the docs
Tests passing before merging the PR: