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
Blocks: add a new Cookie Consent Block #29197
Conversation
Replay of #29191 in the Jetpack plugin. This adds a new Beta block, to be used in block themes, to display a cookie consent banner on the site. Co-authored-by: Omar Alshaker <omar@omaralshaker.com>
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run
to get started. More details: p9dueE-5Nn-p2 |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
projects/plugins/jetpack/extensions/blocks/cookie-consent/cookie-consent.php
Outdated
Show resolved
Hide resolved
* @return bool | ||
*/ | ||
function should_register_block() { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is there for development, it should be removed before we merge this PR.
attributes, | ||
example: { | ||
attributes: { | ||
// @TODO: Add default values for block attributes, for generating the block preview. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to address that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to add anything here if the block as a default value for every attribute?
source: 'html', | ||
selector: 'p', | ||
default: __( | ||
'Privacy & Cookies: This site uses cookies. By continuing to use this website, you agree to their use. To find out more, including how to control cookies, see here: <a href=https://automattic.com/cookies/>Cookie Policy</a>.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that self-hosted sites would need a different link here. Maybe Atomic too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for the widget we allow custom links in there, and also take into account the page you can set as your privacy policy page in WordPress:
jetpack/projects/plugins/jetpack/modules/widgets/class-jetpack-eu-cookie-law-widget.php
Lines 138 to 139 in 3104e37
'default-policy-url' => 'https://automattic.com/cookies/', | |
'custom-policy-url' => get_option( 'wp_page_for_privacy_policy' ) ? get_permalink( (int) get_option( 'wp_page_for_privacy_policy' ) ) : '', |
This is something we could improve on in a follow-up PR I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a nice improvement for the default value, but the block is entirely customizable and users can change the URL.
projects/plugins/jetpack/extensions/blocks/cookie-consent/index.js
Outdated
Show resolved
Hide resolved
Thank you so much @jeherve and @anomiex! I addressed all your feedback.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a try. I think this first iteration is ready to merge; we can iterate in follow-up PRs.
I cannot approve it though since I opened the PR, so I'll let someone else from Crew take care of that.
I left some more comments below.
I wonder if some folks will be tempted to do something like this:
And if so, should we support having 2 buttons, one accept, one reject? That's probably something for a v2.
}, | ||
], | ||
] } | ||
templateLock="all" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if I had more flexibility over the button, such as alignment within the block. Maybe we could give more options by not locking the button?
Since the button has to be there for the block to work, we could maybe take an approach similar to #24838?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually started with a much more flexible block, we had the whole thing as a template (see: pdDR7T-Cr-p2) but the flexibility makes it a great tool to abuse and show one-time ads on your site.
The only reason I see InnerBlocks
here is that I couldn't figure hard-coding a customizable button. RichText
only allowed us to hard-code a p
but I was unable to find a way for a button.
allowedBlocks={ [ 'core/button' ] } | ||
template={ [ | ||
[ | ||
'core/button', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we would get more options by switching to using jetpack/button
, like we do in most of our other blocks that use buttons?
text: 'var(--wp--preset--color--contrast)', | ||
background: 'var(--wp--preset--color--tertiary)', | ||
link: 'var(--wp--preset--color--contrast)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't come up with a way to make this work. Undocumented and I tried everything I can come up with!
projects/plugins/jetpack/extensions/blocks/cookie-consent/index.js
Outdated
Show resolved
Hide resolved
{ | ||
label: isRTL() ? __( 'Right', 'jetpack' ) : __( 'Left', 'jetpack' ), | ||
value: 'left', | ||
}, | ||
{ | ||
label: __( 'Full', 'jetpack' ), | ||
value: 'full', | ||
}, | ||
{ | ||
label: __( 'Wide', 'jetpack' ), | ||
value: 'wide', | ||
}, | ||
{ | ||
label: isRTL() ? __( 'Left', 'jetpack' ) : __( 'Right', 'jetpack' ), | ||
value: 'right', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we offer a center option here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took us so much work to get this running in more than one theme.
onChange={ textValue => setAttributes( { text: textValue } ) } | ||
/> | ||
<InnerBlocks | ||
allowedBlocks={ [ 'core/button' ] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we allow headings here as well, for folks who want to add a bigger title before the explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These InnerBlocks
are only around the button. They're not meant to give control to the user. They're just a way to make the button customizable. I wish I could lock it down further.
projects/plugins/jetpack/extensions/blocks/cookie-consent/edit.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed some improvements and it looks great now!
projects/plugins/jetpack/extensions/blocks/cookie-consent/cookie-consent.php
Outdated
Show resolved
Hide resolved
attributes, | ||
example: { | ||
attributes: { | ||
// @TODO: Add default values for block attributes, for generating the block preview. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to add anything here if the block as a default value for every attribute?
source: 'html', | ||
selector: 'p', | ||
default: __( | ||
'Privacy & Cookies: This site uses cookies. By continuing to use this website, you agree to their use. To find out more, including how to control cookies, see here: <a href=https://automattic.com/cookies/>Cookie Policy</a>.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a nice improvement for the default value, but the block is entirely customizable and users can change the URL.
}, | ||
], | ||
] } | ||
templateLock="all" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually started with a much more flexible block, we had the whole thing as a template (see: pdDR7T-Cr-p2) but the flexibility makes it a great tool to abuse and show one-time ads on your site.
The only reason I see InnerBlocks
here is that I couldn't figure hard-coding a customizable button. RichText
only allowed us to hard-code a p
but I was unable to find a way for a button.
text: 'var(--wp--preset--color--contrast)', | ||
background: 'var(--wp--preset--color--tertiary)', | ||
link: 'var(--wp--preset--color--contrast)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't come up with a way to make this work. Undocumented and I tried everything I can come up with!
{ | ||
label: isRTL() ? __( 'Right', 'jetpack' ) : __( 'Left', 'jetpack' ), | ||
value: 'left', | ||
}, | ||
{ | ||
label: __( 'Full', 'jetpack' ), | ||
value: 'full', | ||
}, | ||
{ | ||
label: __( 'Wide', 'jetpack' ), | ||
value: 'wide', | ||
}, | ||
{ | ||
label: isRTL() ? __( 'Left', 'jetpack' ) : __( 'Right', 'jetpack' ), | ||
value: 'right', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took us so much work to get this running in more than one theme.
onChange={ textValue => setAttributes( { text: textValue } ) } | ||
/> | ||
<InnerBlocks | ||
allowedBlocks={ [ 'core/button' ] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These InnerBlocks
are only around the button. They're not meant to give control to the user. They're just a way to make the button customizable. I wish I could lock it down further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go with Jeremy saying it's good enough to merge and iterate on.
Proposed changes:
Replay of #29191 in the Jetpack plugin.
This adds a new Beta block, to be used in block themes, to display a cookie consent banner on the site.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
eucookielaw
) when you click to accept the banner. That cookie is already in use by the Cookies & Consents Banner widget, but that should still be documented.Testing instructions:
add_filter( 'jetpack_blocks_variation', function () { return 'beta'; } );
).eucookielaw
cookie be added in your browser. The banner should then be gone.