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

Add support for https://github.com/orestbida/cookieconsent v3 #85

Merged
merged 4 commits into from
Oct 21, 2022

Conversation

meixger
Copy link
Contributor

@meixger meixger commented Oct 5, 2022

@OhMyGuus OhMyGuus self-requested a review October 5, 2022 15:21
@OhMyGuus
Copy link
Owner

OhMyGuus commented Oct 19, 2022

Sorry for the late response and thanks for the contribution & PR!
I totally missed the V3 part so was a bit confused, I now found the V3 version and tested it. It seems to work Only a bit scared of the downside of using cc-main since the id is a bit abstract and could break some websites.

I would rather have chosen for
.show--consent #cc-main
or
#cc-main .cm-wrapper (so it blocks cm-wrapper inside of cc-main)

But thanks a lot!

For later reference the demo for v3:
https://playground.cookieconsent.orestbida.com/

@meixger
Copy link
Contributor Author

meixger commented Oct 20, 2022

the downside of using cc-main since the id is a bit abstract and could break some websites

Yes, indeed. We should match on html.show--consent #cc-main.


But i missed that the option disablePageInteraction disables the vertical scroll bar:

html.disable--interaction.show--consent, html.disable--interaction.show--consent body {
    height: auto!important;
    overflow: hidden!important;
}

I found no way to override the cookieconsent !important styles. Do you have any guidance?

@OhMyGuus
Copy link
Owner

OhMyGuus commented Oct 20, 2022

html.show--consent #cc-main

Looked in

the downside of using cc-main since the id is a bit abstract and could break some websites

Yes, indeed. We should match on html.show--consent #cc-main.

But i missed that the option disablePageInteraction disables the vertical scroll bar:

html.disable--interaction.show--consent, html.disable--interaction.show--consent body {
    height: auto!important;
    overflow: hidden!important;
}

I found no way to override the cookieconsent !important styles. Do you have any guidance?

mmh 🤔 it seems that there isn't a easy way for this at the moment, I think the best way at the moment is to add

.cm__btn[data-role=necessary],\

to the searchGroups in common.js with the css html.show--consent #cc-main which will hide the banner & reject it in the background which will remove the scrollbar.

edit: Found another way but idk which one is better in this case #126

@meixger
Copy link
Contributor Author

meixger commented Oct 20, 2022

.cm__btn[data-role=necessary],\

Unfortunately data-role=necessary is not reliable enough because it could be hidden by the web developer. (I'm guilty of such a use case 😳)
In such case the only possibility to exit the modal dialog would be the button with data-role=all.

window.CookieConsent.reset() would close the consent dialog without accepting anything.

@OhMyGuus
Copy link
Owner

OhMyGuus commented Oct 20, 2022

Thanks 😄 took all the thoughts and it should work fine now. It will block the popup using
html.show--consent #cc-main
Then if disablePageInteraction is enabled it will click the button
.disable--interaction .cm__btn[data-role=necessary]'
and if the button doesn't exist it will fall back to the css rule which has been fixed by updating the priority :

html.disable--interaction.show--consent,
html.disable--interaction.show--consent body {
  overflow: unset !important;
}

I might create a custom Javascript hander later for things like window.CookieConsent.reset() but that was a bit too much work for the current release 👍

@OhMyGuus OhMyGuus merged commit 9821317 into OhMyGuus:master Oct 21, 2022
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.

None yet

2 participants