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

Show/Hide elements based on consent #41

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

timoklok
Copy link
Contributor

After refactoring this module, the functionality of showing/hiding elements based on consent wasn't working anymore. Apparently this feature doesn't seem to be used at the moment, since this is usually handled by the website that implements this module, by looking at changes in the consent.

Nevertheless, the code in this module didn't work, so I fixed it, and also added tests for it, and cleaned up some code that wasn't doing anything useful.

AC-728

After refactoring this module, the functionality of showing/hiding elements based on consent wasnt working anymore. However, this feature doesnt seem to be used at the moment, since this usually handled by the website that uses this module, by looking at changes in the consent.

Nevertheless, the code in this module didn't work, so I fixed it, and also added tests for it, and cleaned up some code that wasnt doing anything usefull.

AC-728
@timoklok timoklok force-pushed the feature/timo-ac728-show-content-on-consent branch from 72eea63 to 8a70477 Compare January 16, 2025 08:56
@@ -49,8 +49,6 @@ export default class Dialog extends HTMLElement {
// initialize show and hide
this.show = this.show();
this.hide = this.hide();
// get all preferences
this.preferences.getAll();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getAll() functions returns preferences. Running it without assigning it to a variable doesn't do anything.

@@ -214,7 +210,7 @@ export default class Dialog extends HTMLElement {
}

initDomToggler() {
return DomToggler(Config());
return DomToggler(this.cookies);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the DomToggler was called with the wrong property. Config() doesn't actually return infomartion about the current state of cookies, only initial configuration data. Therefor, the Domtoggler couldn't show or hide elements based on consent, since there was no info about consent.

@@ -103,7 +103,7 @@ const DomToggler = (config) => {

return {
toggle: (preferences) => {
const cookies = config.get("cookies") || [];
const cookies = cookieData || [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment


expect(cookieConsent.preferences.getAll("foo")).toEqual(input);
cookieConsent.updatePreference(COOKIES);
expect(cookieConsent.preferences.getAll()).toEqual(COOKIES);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test included code that didn't actually do anything. When the module is initialized, it actually doesn't set any localStorage values about cookies, that only happens when there has been interaction with the module.

So the test is also valid by just calling updatePreferences a single time.

Copy link
Member

@MicheleNL MicheleNL left a comment

Choose a reason for hiding this comment

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

LGTM! Ik heb dit lokaal nog even getest, want ik had de package toch al ergens lokaal gelinkt, en dit lijkt goed te werken. Wat fijn Timo!

@timoklok timoklok merged commit 2a299df into master Jan 18, 2025
4 checks passed
@timoklok timoklok deleted the feature/timo-ac728-show-content-on-consent branch January 18, 2025 09:55
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