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

137-consent-prompt-for-video-embed-block #138

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

luukbrauckmann
Copy link
Member

@luukbrauckmann luukbrauckmann commented May 1, 2024

Changes

  • Adds consent alert to VideoEmbedBlock

Associated issue

Resolves #137

How to test

  1. Open preview link
  2. Scroll down to the Video
  3. Click on the video
  4. There should be an overlay popping up asking for permission
  5. Click the allow button.
  6. Video should start.
  7. Check if this permission is added in the local storage.

Checklist

  • I have performed a self-review of my own code
  • I have made sure that my PR is easy to review (not too big, includes comments)
  • I have made updated relevant documentation files (in project README, docs/, etc)
  • I have added a decision log entry if the change affects the architecture or changes a significant technology
  • I have notified a reviewer

@luukbrauckmann luukbrauckmann linked an issue May 1, 2024 that may be closed by this pull request
Copy link

cloudflare-workers-and-pages bot commented May 1, 2024

Deploying head-start with  Cloudflare Pages  Cloudflare Pages

Latest commit: cc2638d
Status: ✅  Deploy successful!
Preview URL: https://bfbf6b0f.head-start.pages.dev
Branch Preview URL: https://137-consent-prompt-for-video.head-start.pages.dev

View logs

@luukbrauckmann
Copy link
Member Author

I could pretty easily copy all the code from the nododos project, because it's pretty generic.

@luukbrauckmann luukbrauckmann marked this pull request as ready for review May 1, 2024 13:54
@luukbrauckmann
Copy link
Member Author

Readme already contained an item about the consent alert

@luukbrauckmann luukbrauckmann requested a review from decrek May 1, 2024 13:58
@jbmoelker
Copy link
Member

👏👏. We should do the same for the Embed Block, but probably in a new PR :)

Copy link
Member

@decrek decrek left a comment

Choose a reason for hiding this comment

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

Why do we store the user's choice in local storage and not in a cookie? If we would do server side rendering we could render/not render the cookie preferences. I understand that that doesnt make sense for the video embed, but for cookie cookie preferences in general I would like a solution that saves the preferences in a cookie

@jbmoelker jbmoelker merged commit 575cf4e into main Jun 21, 2024
4 checks passed
@jbmoelker jbmoelker deleted the 137-consent-prompt-for-video-embed-block branch June 21, 2024 14:24
@jbmoelker
Copy link
Member

👏👏. We should do the same for the Embed Block, but probably in a new PR :)

See #157

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.

Consent prompt for video embed block
3 participants