Added Event-Handler Arguments for Inserted & Closed

[wottpal]

Hey there, I just added two event-handler arguments. Hope this is fine! Only adds ~8 small lines of code. I also described them in the README.

Regards from Germany, Dennis

[wottpal]

Example:

var options = {
  insertedHandler: function (el) {
      console.log("Hey there I got added.", el);
  }, 
  closedHandler: function () {
      console.log("I got closed");
  }
};
var cb = new Cookiebanner(options); cb.run();

[zytzagoo]

Hey, thanks for the contribution!

Here's a few quick thoughts based on a cursory look:

• I'm not sure the new option names are consistent with the existing ones, but we'll see what others say. I'd personally call them 'callback-inserted', 'callback-closed' or 'on-inserted', 'on-closed' or something like that -- and we should also check what happens when those options are set via data-attributes (if we're going to allow/support that).

• The minified version is created and tagged/committed as part of the release process, so no need to include that in the PR.

• Would you be willing to add some tests that cover these new features/options?

Let's see what others think!

[wottpal]

Hey, thanks for reviewing my PR! I improved the following things:

• Renamed handlers
• Make it possible to add handlers as data-attributes
• Added a test for each handler
• Added a section in the README which describes the two ways to utilize handlers

Dennis :)

[wottpal]

[zytzagoo]

Love the changes and the tests, great job!

We must resolve conflicts tho, can you rebase this against latest master?

And there's a breaking change in there with regards to old IE (due to String.prototype.startsWith), but I can take care of that later.

[wottpal]

I hope I've done this right, never done this before 😬

[zytzagoo]

That's one way of doing it, sure. Thanks.

Could you also take care of the linting errors as reported by travis?

[wottpal]

Yeah I fixed most of them. Only one about eval() can be harmful is still there but imo there is no other way to parse code from data-attributes. So either we ignore the warning and let the user assure no harmful code is inserted into their data-attributes or we make handlers configurable via JS only.

[zytzagoo]

We can add a trailing comment instructing jshint to ignore that line if eval is to stay?

    ...; // jshint ignore:line

Or, further investigate the use of new Function to create handlers (instead of eval), but it would complicate things some more (and not really change things too much, apart from preventing handler's access to lexical scope? We'd still have to instruct jshint to ignore the line if I remember correctly)

I think having handlers defined in HTML could be handy for certain cases, even if it brings some caveats due to eval usage...

Pinging @senko to hear his thoughts...

[wottpal]

We can add a trailing comment instructing jshint to ignore that line if eval is to stay? ✅

Travis is still seeing a failing test but for me in Chrome all tests are just fine 🤷‍♂️

[zytzagoo]

Yeah, restarted the travis build quickly to see if it'll happen again, and it did.

Not sure what's going on in there now... let me pull down and checkout the PR locally to dig in...

[zytzagoo]

Tests were failing due to modern ES syntax.

I also spotted a few minor details that I tweaked:

• argument given/passed to event handlers/callbacks were not actually being passed, and yet were present in the README, so I decided to pass the Cookiebanner instance as the first argument of both callbacks.
• changed syntax in tests.js + added tests to cover the arguments passed to handlers
• updated README slightly

I think we can merge this now, and then take care of String.startsWith replacement for older browsers (old IE is still supported kindof, since it worked so far) + changelog entries and whatnot...

Thanks a ton for your work on this!