search

grrr-amsterdam / cookie-consent

Cookie consent with accessible dialog, agnostic tag triggers and conditional content, script and embed hooks.
MIT License
63 stars 11 forks source link

Fix "Child to insert before is not a child of this node" #14

Closed Mte90 closed 3 years ago

Mte90 commented 3 years ago

I am getting this if I compile the library and using in a page with main available.

The website is https://glossary.codeat.co That patch let me to use the library with no issues otherwise there was that error. Probably can be improved for the tests.

harmenjanssen commented 3 years ago

Ah, I see, it's because your main element is not a direct child of body. I understand the problem, and I think your change is a good solution.

To make sure everything keeps working, I would like to make sure the tests pass, but after that I will welcome this change!

Mte90 commented 3 years ago

Seems that the build didn't started.

martijngastkemper commented 3 years ago

Weird. I've restarted the build.

Mte90 commented 3 years ago

Ok this time worked and everything is red.

harmenjanssen commented 3 years ago

You mean green, but yeah, it is. 😉

But I'm not entirely convinced yet: I think it would be good to throw an exception when appendEl is not defined. That's clearly an error, and right now it's failing silently and the developer will have no idea why this isn't working.

Throwing an exception would clearly signal what's going wrong. Do you agree?

Mte90 commented 3 years ago

Ops yes green, too many things in parallel.

So you want a console.log?

harmenjanssen commented 3 years ago

A full-on exception would have my preference:

throw new Error("Unable to add cookie dialog: element does not exist.");
Mte90 commented 3 years ago

Added but I guess that now the tests suite will alert that there is no dialog.

harmenjanssen commented 3 years ago

It's a bit weird how the tests didn't throw the error you were trying to solve in this PR.
I'll have to think about why that is, I don't understand why the test either didn't fail before, or doesn't succeed now.

Mte90 commented 3 years ago

done

harmenjanssen commented 3 years ago

I've published under version 1.0.4.