-
Notifications
You must be signed in to change notification settings - Fork 240
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
Youtube Click To Load - UI Implementation #1433
Youtube Click To Load - UI Implementation #1433
Conversation
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 looks great, thanks 🥇.
I've left a bunch of comments, mostly nits. But IMO let's focus on getting it into a state fit for landing (close as possible, without breaking any existing functionality), so we can do the remaining follow-up work later when one of us has time.
I'm going to test this now, will comment back if I notice any issues.
@franfaccin Oh, I've noticed this is breaking the integration tests too. I recommend running those locally with the |
To get the integration tests running a bit more, you just need to update the integration test configuration: diff --git a/integration-test/data/configs/click-to-load-youtube.json b/integration-test/data/configs/click-to-load-youtube.json
index 558f6497..83c6f7a0 100644
--- a/integration-test/data/configs/click-to-load-youtube.json
+++ b/integration-test/data/configs/click-to-load-youtube.json
@@ -37,10 +37,19 @@
],
"replaceSettings": {
"type": "youtube-video-ctl",
- "buttonText": "Unblock Video",
- "infoTitle": "DuckDuckGo blocked this video to prevent YouTube from tracking you",
- "infoText": "We blocked YouTube from tracking you when the page loaded. If you unblock this Video, YouTube will know your activity.",
- "simpleInfoText": "We blocked YouTube from tracking you when the page loaded. If you unblock this Video, YouTube will know your activity."
+ "buttonText": "Unblock video",
+ "infoTitle": "DuckDuckGo blocked this YouTube video to prevent Google from tracking you",
+ "infoText": "We blocked Google (which owns YouTube) from tracking you when the page loaded. If you unblock this video, Google will know your activity.",
+ "simpleInfoText": "We blocked Google (which owns YouTube) from tracking you when the page loaded. If you unblock this video, Google will know your activity.",
+ "previewToggleText": "Previews disabled for additional privacy",
+ "placeholder": {
+ "previewToggleEnabledText": "Previews enabled",
+ "previewInfoText": "Turn previews off for additional privacy from DuckDuckGo.",
+ "videoPlayIcon": {
+ "lightMode": "video_play_light.svg",
+ "darkMode": "video_play_dark.svg"
+ }
+ }
},
"clickAction": {
"type": "youtube-video-ctl"
@@ -59,9 +68,9 @@
}
},
"informationalModal": {
- "icon": "blocked_video.svg",
+ "icon": "blocked_youtube_video.svg",
"messageTitle": "Enable YouTube previews and reduce privacy?",
- "messageBody": "Showing previews will allow Google (which owns YouTube) to see some of your device’s information, but is still more private than playing the video. Learn More",
+ "messageBody": "Showing previews will allow Google (which owns YouTube) to see some of your device’s information, but is still more private than playing the video.",
"confirmButtonText": "Enable Previews",
"rejectButtonText": "No Thanks"
}, But you'll see some tests are still failing. Those failures are real, the requests to Google for the video details are still being made in the background, even when video previews are disabled! I'm not sure what's causing that though, I don't have time to look into it. Edit: Also, please could you add a TODO comment in the YouTube CTL integration tests, to mention that we need to add tests for enabling previews? |
acfd596
to
6a441f2
Compare
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.
Thanks for addressing all of those comments.
I've looked into the test failures briefly again today and tested out your changes. Two things:
- Some tests are correct to fail, since we are making requests to YouTube that are unexpected. It seems that even when video previews are disabled, we're still fetching the video details.
- Not related to the test failures, but clicking to unblock a video when previews are disabled results in the video auto-playing. I don't think that's correct, I thought videos should only auto-play if previews are enabled, but please correct me if we changed the decision on that.
How to verify above:
- Do fresh install of the extension in Chrome (previews should be disabled by default).
- Navigate to
chrome://extensions
, select to inspect the background page for the extension, switch to the network panel there. - In a new tab load the test page.
- You'll see requests to
https://www.youtube.com/oembed
in the background page even though previews are disabled. - Click to unblock a video, notice how it auto-plays.
Shout when you've addressed the above and I'll take a more careful pass of the changes + have another look at any remaining test failures!
caa62bf
to
e4c4c24
Compare
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.
LGTM 👍
Reviewer:
CC: @kzar
Description:
Test
Build steps: https://github.com/duckduckgo/duckduckgo-privacy-extension/blob/develop/CONTRIBUTING.md#building-the-extension
Steps to test this PR:
Automated tests:
Reviewer Checklist:
PR Author Checklist: