#579: Nieuwe release maken Cookie Consent module #39

CountNick · 2024-06-17T14:45:05Z

Update the readme
- Since the cookie-consent is now a custom element the readme has been adjusted accordingly to be used in a vanilla js project. The old module had the option to choose between radio buttons and checkboxes. There is no option to choose anymore it will always be a list of checkboxes right now. Is this something that needs to be re-added?
Update the default stylesheet
- Since this is now a custom element, the html is rendered in a shadow DOM therefore it won't be possible to just style the element from the outside with class names. This now has to be done with specific ::part's I've updated the default stylesheet to now use those.
Update the save preferences button
- I noticed the button always showed the default value, it's now been adjusted to be able to show custom text as well.

harmenjanssen

Thanks for preparing the release, Nick!
I feel like there are a couple small things to look into before we take this out of beta.

Please let me know if any comments are unclear or if you want to look into something together.

harmenjanssen · 2024-06-18T05:24:53Z

README.md

-
-window.CookieConsent = cookieConsent;
+export const DEFAULTS = {
+    type: "checkbox",


Here "type: checkbox" is still listed.
If we don't support otherwise, I would remove this option.

This has now been removed

harmenjanssen · 2024-06-18T05:25:42Z

README.md

-el.addEventListener('click', e => {
-  e.preventDefault();
-  cookieConsent.hideDialog();
+el.addEventListener("click", (e) => {


In the previous example you use button.addEventListener, I would do so here as well.

This has been changed to button.addEventListener

harmenjanssen · 2024-06-18T05:27:08Z

README.md

+/**
+ * Header
+ */
+wookie-consent::part(cookie-consent__header) {


😂 Wookie-consent!

Haha oops, this has been adjusted to cookie-consent

harmenjanssen · 2024-06-18T05:28:41Z

README.md

 });
 ```

-### isAccepted(id: string)


What happened to all methods that have been removed from the README?
They all sound very useful. If they are gone, I would like to make some GH issues to remind us to re-implement them.
(talking about isAccepted, getPreferences, on, and updatePreferences)

From what I can see in dialog.mjs the methods are still available, so I would like to return the documentation on them as well.

Ah yeah, I didn't see. Everything has been re-added except for isAccepted this method is not being used anymore as this can be easily checked in the frontend right now after cookies have been updated.

harmenjanssen · 2024-06-18T05:53:22Z

src/dialog.mjs

+    // sets and returns 'this.shadowRoot'
+    this.attachShadow({ mode: "open" });
+    // get data
+    this.data = this.getData();
+    // get config
+    this.config = this.getConfig();
+    // initialize event dispatcher
+    this.events = this.initEventDispatcher();
+    // initialize teblist
+    this.tabList = this.initTabList();
+    // get cookies
+    this.cookies = this.data.cookies;
+    // generate dialog element
+    this.dialogElement = this.generateDialogElement();
+    // append dialog to shadowRoot
+    this.shadowRoot.append(this.dialogElement);
+    // get preferences
+    this.preferences = this.getPreferences();
+    // initialize domtoggler
+    this.domToggler = this.initDomToggler();
+    // initialize show and hide
+    this.show = this.show();
+    this.hide = this.hide();
+    // get all preferences
+    this.preferences.getAll();


I don't think the comments add a lot of value here, they're nearly copies of the actual lines of code.

What would you suggest, should they be removed or rephrased? 🙂

I think they can be safely removed!

harmenjanssen · 2024-06-18T05:55:09Z

src/dialog.mjs


    // Radio when no option is selected.
-    if (TYPE === 'radio' && !values.find(v => v.accepted)) {
+    if (this.config.type === "radio" && !values.find((v) => v.accepted)) {


Here are also still references to radiobuttons.

This is now gone

harmenjanssen · 2024-06-18T05:58:47Z

test/cookie-consent.test.js

I would prefer to leave this test intact.
It's a really simple test, feels doable to reproduce it for the new module version.

The test is now rewritten to work with the custom element

harmenjanssen · 2024-06-18T06:00:29Z

src/dialog.mjs

Can we rename this to CookieConsent? (cookie-consent.mjs)
It makes sense to me that the "main" element in this module is called "cookie consent" and not "dialog". It feels more intuitive to the developers working on this.

harmenjanssen · 2024-06-18T06:03:08Z

Oh, and regarding the checkboxes versus radiobuttons: can you elaborate on why the radiobuttons are no longer supported?

If we don't use them in client projects it's fine to move ahead with this, but let's in any case make an issue here on GitHub to keep track of the status of that feature.

JairoTheDev

@harmenjanssen is zo te zien al bezig geweest! Ik heb ook nog paar kleine comments.

JairoTheDev · 2024-06-18T06:16:58Z

README.md

- When showing, the module will remove any inline set `display` style, along with any `hidden` or `aria-hidden` attributes.
-
-## Options
+Once registered, you can add the cookie-consent element to your HTML, you can pass custom data attributes for the:


Deze zin loopt niet helemaal lekker! Misschien kan je het beter veranderen in:

Suggested change

Once registered, you can add the cookie-consent element to your HTML, you can pass custom data attributes for the:

Once registered, you can add the cookie-consent element to your HTML and pass custom data attributes for the following:

Ah thanks! Dit is aangepast

JairoTheDev · 2024-06-18T06:20:48Z

README.md

+-   title
+-   description
+-   save button text
+-   cookie types

 ```js


Dit loopt nog niet helemaal lekker in elkaar over. De data attributes worden benoemd, maar dit wordt pas verder uitgelegd onder het kopje Options. Misschien kan dit allemaal verplaatst worden on het kopje Options.

ook dit is nu wat aangepast

CountNick · 2024-06-18T09:59:29Z

Oh, and regarding the checkboxes versus radiobuttons: can you elaborate on why the radiobuttons are no longer supported?

If we don't use them in client projects it's fine to move ahead with this, but let's in any case make an issue here on GitHub to keep track of the status of that feature.

The radio buttons are no longer supported because the necessary cookies should always be selected and therefore any other option added should also be selectable. A list of checkboxes would mean only one option could be selected

harmenjanssen · 2024-06-18T12:45:38Z

Oh, and regarding the checkboxes versus radiobuttons: can you elaborate on why the radiobuttons are no longer supported?
If we don't use them in client projects it's fine to move ahead with this, but let's in any case make an issue here on GitHub to keep track of the status of that feature.

The radio buttons are no longer supported because the necessary cookies should always be selected and therefore any other option added should also be selectable. A list of checkboxes would mean only one option could be selected

But are there always required cookies? In the past this could be used with all-optional cookies I think.

harmenjanssen

Nice, Nick, thanks for the updates!
This is starting to look good.

Can you look into the failing CI processes?

I would prefer to have a little more tests, just to have some affordance that this update does indeed keep existing functionality intact, but since there were so little to begin with, we can get away with moving them to the backlog and work on them in retrospect.
But for instance, a check that the public methods still work would be very nice to have.

CountNick · 2024-06-19T07:01:55Z

Oh, and regarding the checkboxes versus radiobuttons: can you elaborate on why the radiobuttons are no longer supported?
If we don't use them in client projects it's fine to move ahead with this, but let's in any case make an issue here on GitHub to keep track of the status of that feature.

The radio buttons are no longer supported because the necessary cookies should always be selected and therefore any other option added should also be selectable. A list of checkboxes would mean only one option could be selected

But are there always required cookies? In the past this could be used with all-optional cookies I think.

To be fair I don't really know, what I do know is that it's legal to always have functional cookies on. But then again I don't really know whether or not they're always necessary.

CountNick · 2024-06-19T07:57:12Z

@harmenjanssen all CI processes are now succeeding except for the slack notification is this something that needs to be fixed as well?

CountNick · 2024-06-19T08:51:08Z

@harmenjanssen the slack notification is now also succeeding

harmenjanssen

LGTM!

Add config functions Update code Add data attributes Update code Update custom events Update template Remove redundant code Fix show and hide dialog functionality Remove old code Revert to .mjs Update formatting Update imports Update import Remove @types/react Update dialog Update dialog Update tablist Add parameters to function Remove redundant file Fix linter error Update test Update checked state Remove whitespaces Update yarn lock Fix linting errors Remove failing test Remove aria-description 1.2.1-beta.1 Update cookie-consent to custom element Add config functions Update code Add data attributes Update code Update custom events Update template Remove redundant code Fix show and hide dialog functionality Remove old code Revert to .mjs Update formatting Update imports Update import Remove @types/react Update dialog Update dialog Update tablist Add parameters to function Remove redundant file Fix linter error Update test Update checked state Remove whitespaces Update yarn lock Fix linting errors Remove failing test Remove aria-description Hide cookie consent in print media styles Remove idea files Remove idea files Remove idea workspace file Update dialog generateDialogElement method Update imports with file extension Update package version grrr utils Updated defaultbuttonLabel to defaultButtonLabel Update README Update README Update README Update button label Update stylesheet Update readme Update code Update readme Update string quotes Re-add test Format code Update slack notification Format code Update version Format files

CountNick requested review from harmenjanssen, roelandvs and JairoTheDev June 17, 2024 14:45

CountNick changed the title ~~Feature/update readme~~ #579: Nieuwe release maken Cookie Consent module Jun 17, 2024

harmenjanssen reviewed Jun 18, 2024

View reviewed changes

JairoTheDev requested changes Jun 18, 2024

View reviewed changes

CountNick requested review from harmenjanssen and JairoTheDev June 18, 2024 12:43

harmenjanssen reviewed Jun 18, 2024

View reviewed changes

CountNick requested a review from harmenjanssen June 19, 2024 08:51

harmenjanssen approved these changes Jun 19, 2024

View reviewed changes

CountNick force-pushed the feature/update-readme branch from 34785e1 to 86dc5a3 Compare June 20, 2024 07:59

CountNick force-pushed the feature/update-readme branch from 7e61f25 to 403bd7c Compare June 20, 2024 09:39

CountNick merged commit 72d9319 into master Jun 20, 2024
4 checks passed

CountNick deleted the feature/update-readme branch June 20, 2024 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#579: Nieuwe release maken Cookie Consent module #39

#579: Nieuwe release maken Cookie Consent module #39

CountNick commented Jun 17, 2024

harmenjanssen left a comment

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen Jun 18, 2024

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen Jun 18, 2024

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen Jun 18, 2024

CountNick Jun 18, 2024

harmenjanssen commented Jun 18, 2024

JairoTheDev left a comment

JairoTheDev Jun 18, 2024

CountNick Jun 18, 2024

JairoTheDev Jun 18, 2024

CountNick Jun 18, 2024

CountNick commented Jun 18, 2024

harmenjanssen commented Jun 18, 2024

harmenjanssen left a comment

CountNick commented Jun 19, 2024

CountNick commented Jun 19, 2024

CountNick commented Jun 19, 2024

harmenjanssen left a comment

	Once registered, you can add the cookie-consent element to your HTML, you can pass custom data attributes for the:
	Once registered, you can add the cookie-consent element to your HTML and pass custom data attributes for the following:

#579: Nieuwe release maken Cookie Consent module #39

#579: Nieuwe release maken Cookie Consent module #39

Conversation

CountNick commented Jun 17, 2024

harmenjanssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmenjanssen commented Jun 18, 2024

JairoTheDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CountNick commented Jun 18, 2024

harmenjanssen commented Jun 18, 2024

harmenjanssen left a comment

Choose a reason for hiding this comment

CountNick commented Jun 19, 2024

CountNick commented Jun 19, 2024

CountNick commented Jun 19, 2024

harmenjanssen left a comment

Choose a reason for hiding this comment