-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature Reopen cookie notice #50
Feature Reopen cookie notice #50
Conversation
dist/scripts/cookie-consent.js
Outdated
document.body.classList.add('has-cookie-consent'); | ||
showCookieNotice(); | ||
var button = document.querySelector('[data-trigger-cookie-consent]'); | ||
console.log(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.
debug code should be removed 😬 I also wonder why it's added in the dist script only 🤔 maybe added manually?
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
@@ -3,21 +3,34 @@ | |||
return; | |||
} | |||
|
|||
function addCookieSettingsButton() { |
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.
The function name is misleading as this function doesn't actually add a button but registers the click handler for it. IMO this doesn't necessarily needs to be in a dedicated function as it's invoked once. But we can leave it to move forward 👍
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.
referenced in issue and created a ticket for it to fast forward. #51
…when in hidden state.
Ticket
Description