search

dobarkod / cookie-banner

JavaScript based cookie-info banner for complying with EU cookie law
MIT License
425 stars 84 forks source link

Identify the link with its class instead of the tag name. #83

Closed stormleoxia closed 5 years ago

stormleoxia commented 5 years ago

If the message contains a link, the current code will take the first link and so it won't be the right one. My current proposal and solution is to add a class and find the link with its class.

zytzagoo commented 5 years ago

Hi, thanks for contributing!

Would you mind adding tests covering existing and new behaviour so that we can prove everything still works without any unexpected consequences? (e.g, even for existing users that might have links inside their message -- I assume they'd expect the first link to be used as is/was currently)?

Otherwise, this can only be released along with a major version bump (and an explanation of the [potentially] breaking change in the Readme/changelog).

Or am I overthinking this?

Anyways, perhaps making this behaviour optional (while defaulting to existing behaviour) could be a way to get it merged (and released) more quickly, while avoiding the backcompat issue? That would, however, require some more changes... As there are multiple ways that could be achieved etc.

stormleoxia commented 5 years ago

Hello, happy to contribute,

I've reverted back to the old behavior by default and added a moreinfoClass parameter. When provided, it will be used to detect the moreinfo link.

I've added two tests. One on backward compatibility and one on the new behavior when the parameter is provided.