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

Switch Click to Load to the "clickToLoad" feature name #1763

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

kzar
Copy link
Collaborator

@kzar kzar commented Mar 7, 2023

Initially the Click to Load feature was misnamed "clickToPlay". Also
the Facebook entity name was inconsistently called either "Facebook"
or "Facebook, Inc.". We have corrected those the extension
configuration, content-scope-scripts and the reference tests. Let's
update those dependencies and make the necessary extension changes
here.

Reviewer: @ladamski
CC: @kdzwinel

Testing

  1. Ensure the unit tests still pass.
  2. Ignore if Facebook integration tests are marked pending, Sam has a follow-up to switch to Playwright for those.
  3. Edit the configuration and check the "clickToLoad" feature controls Facebook/YouTube Click to Load, instead of the "clickToPlay" feature.
  4. Smoke test Click to Load still works.

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@kzar kzar force-pushed the correct-ctl-name branch 2 times, most recently from d153657 to bbc7f06 Compare March 7, 2023 16:32
@kzar kzar changed the title WIP - Switch Click to Load to the "clickToLoad" feature name Switch Click to Load to the "clickToLoad" feature name Mar 7, 2023
Initially the Click to Load feature was misnamed "clickToPlay". Also
the Facebook entity name was inconsistently called either "Facebook"
or "Facebook, Inc.". We have corrected those the extension
configuration, content-scope-scripts and the reference tests. Let's
update those dependencies and make the necessary extension changes
here.
@kzar kzar marked this pull request as ready for review March 7, 2023 17:40
@kzar kzar requested a review from ladamski March 7, 2023 17:52
Copy link
Contributor

@ladamski ladamski left a comment

Choose a reason for hiding this comment

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

Code itself looks fine but I've had no luck building this locally, for dev or unit test. I get the following:

  • make[1]: *** No rule to make target shared//content-scope-scripts/node_modules/puppeteer/.local-chromium/mac-1002410/chrome-mac/Chromium.app/Contents/Frameworks/Chromium', needed by build/chrome/dev/public/js/background.js'. Stop.

Have you run into this?

@kzar
Copy link
Collaborator Author

kzar commented Mar 7, 2023

Code itself looks fine but I've had no luck building this locally, for dev or unit test. I get the following:

* make[1]: *** No rule to make target `shared//content-scope-scripts/node_modules/puppeteer/.local-chromium/mac-1002410/chrome-mac/Chromium.app/Contents/Frameworks/Chromium', needed by `build/chrome/dev/public/js/background.js'.  Stop.

Have you run into this?

@ladamski That's probably related to the recent changes to the build system. You might try clearing shared/content-scope-scripts and node_modules/ before running npm install again to see if it helps. That said, I have tested the changes my end.

Copy link
Contributor

@ladamski ladamski left a comment

Choose a reason for hiding this comment

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

Got tests working after deleting shared/content-scope-scripts and unit tests pass. Though I'm still unable to test manually since npm run dev-chrome gets stuck in a copy loop for me, but I don't want to hold this up further due to my build issues.

@kzar kzar merged commit 94eb784 into duckduckgo:main Mar 8, 2023
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