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.

Sign up for GitHub

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

Jump to bottom

AMP Support for Cookies and Consent #15722

Merged
merged 4 commits into from
May 22, 2020
Merged

AMP Support for Cookies and Consent #15722

merged 4 commits into from
May 22, 2020

Conversation

MattGeri
Copy link
Contributor

@MattGeri MattGeri commented May 8, 2020 •
edited
Loading

Adds an amp-consent component when the AMP site is being viewed based on the Cookies and Consent widget settings.

Fixes #14398 (EU Cookie Law Widget)

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This enhances an existing feature.

Testing instructions:

  • Ensure the AMP plugin is active
  • In AMP > Settings, select Transitional mode
  • Enable the extra widgets in Jetpack > Settings > Writing > Widgets
  • Add the Cookies & Consents Banner (Jetpack) widget to a sidebar on your theme

Screenshot 2020-05-08 at 13 34 32

* Customise the widget options as desired * Go to the AMP frontend by adding `?amp` to the end of your site url and ensure the consent banner shows up and is customised as per the options selected on the widget.

Screenshot 2020-05-08 at 13 38 16

Additional considerations

I was able to apply most of the settings from the widget to the AMP version of the widget, however the the AMP Consent tag does now allow us to expire after a certain number of days, unless we do server side consent and not client side.

Additionally, the is an option to accept consent on first scroll or after a certain amount of time has passed. However, with AMP we would be able to hide the notice on the above parameters, but there does not appear to be a way to hide and accept the notice. So even though it disappears, the consent would not have been accepted.

I feel for AMP, we should just force the user to click Accept for the modal to be removed. Here are the options I'm talking about:

Screenshot 2020-05-08 at 13 39 18

Lastly, there are no widget areas on the "Reader" mode template. So it's not possible to add this. widget to that theme unless a user customises the theme and adds widget areas.

@MattGeri
AMP Support for Cookies and Consent
f9a4a90 
* Adds a `amp-consent` component when the AMP site is being viewed based on the widget settings
@MattGeri MattGeri added the AMP label May 8, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented May 8, 2020 •
edited
Loading

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against e4fd43d

@MattGeri MattGeri requested a review from pereirinha May 8, 2020 11:47
@pereirinha
Copy link
Contributor

@MattGeri,

Nice work.
I left you a question, which isn't a blocker.

Thanks

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets labels May 8, 2020
@MattGeri MattGeri added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label May 19, 2020
@MattGeri
Copy link
Contributor Author

@jeherve This is ready for review. I see there is a notice to Please add these new PHP files to PHPCS whitelist in bin/phpcs-whitelist.js for automatic linting: modules/widgets/eu-cookie-law/widget-amp.php

Should this be added? I see the original widget template file is not in there.

@jeherve
Copy link
Member

jeherve commented May 19, 2020

Yeah, you should be able to add the file you modified to

jetpack/bin/phpcs-whitelist.js

Line 1 in db3e471

// If the file path starts with anything like in the array below, it should be linted.

@MattGeri
Copy link
Contributor Author

Yeah, you should be able to add the file you modified to

jetpack/bin/phpcs-whitelist.js

Line 1 in db3e471

// If the file path starts with anything like in the array below, it should be linted.

Cool! I've added it 👍

sergeymitr
sergeymitr previously approved these changes May 19, 2020
View reviewed changes
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels May 19, 2020
@kraftbj kraftbj added this to the 8.6 milestone May 19, 2020
jeherve
jeherve requested changes May 20, 2020
View reviewed changes
modules/widgets/eu-cookie-law.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 20, 2020
jeherve
jeherve requested changes May 20, 2020
View reviewed changes
modules/widgets/eu-cookie-law.php Outdated Show resolved Hide resolved
modules/widgets/eu-cookie-law.php Outdated Show resolved Hide resolved
@jeherve
Copy link
Member

jeherve commented May 20, 2020

Internal reference: D43710-code

jeherve
jeherve approved these changes May 22, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well for me now. Merging!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 22, 2020
@jeherve jeherve merged commit 73d904f into master May 22, 2020
@jeherve jeherve deleted the fix/amp-for-cookies-and-consent branch May 22, 2020 16:26
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 22, 2020
@westonruter westonruter mentioned this pull request May 22, 2020
Open
21 tasks
@jeherve
Copy link
Member

jeherve commented May 22, 2020

r207915-wpcom

jeherve added a commit that referenced this pull request May 26, 2020
@jeherve
Changelog: add #15722
98a35f2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashicus ashicus ashicus left review comments

@pereirinha pereirinha pereirinha left review comments

@jeherve jeherve jeherve approved these changes

@sergeymitr sergeymitr sergeymitr left review comments

Assignees

@MattGeri MattGeri

Labels
AMP [Feature] Extra Sidebar Widgets [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.6
Development

Successfully merging this pull request may close these issues.

Widgets: AMP Compatibility
8 participants
@MattGeri @jetpackbot @pereirinha @jeherve @ashicus @sergeymitr @kraftbj @matticbot