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

Analytics: Remove problematic "blacklist"/"whitelist" language #43395 #43643

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Jun 24, 2020

Changes proposed in this Pull Request

  • Rename isUrlBlacklistedForPerformance to isUrlExcludedForPerformance

Testing instructions

  • Ensure unit tests pass
  • Turn off your browser's "Do Not Track" option, enable ad-tracking config, and give GDPR consent. Then navigate to a page that is excluded from analytics (/log-in) and verify that analytics are blocked by checking isAdTrackingAllowed in the browser console and verifying it is false.

Addresses part of #43395

@matticbot
Copy link
Contributor

@sarayourfriend sarayourfriend mentioned this pull request Jun 24, 2020
21 tasks
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@sarayourfriend sarayourfriend changed the title Analytics: Remove problematic "blacklist"/"whitelist" language #43303 Analytics: Remove problematic "blacklist"/"whitelist" language #43395 Jun 24, 2020
@sarayourfriend sarayourfriend added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 24, 2020
@sarayourfriend sarayourfriend added the [Feature] Tracks Metrics & Monitoring Capturing analytics about user behavior on WordPress.com. label Jun 25, 2020
@sarayourfriend sarayourfriend requested review from sgomes, MicBosi and jsnmoon and removed request for sgomes June 29, 2020 16:33
Copy link
Contributor

@sgomes sgomes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @saramarcondes! The changes look good and it tests well 👍

This seems to be based on a version of the base branch that had a linking issue, which only surfaced as a warning and not an error. That's since been fixed, and I would recommend rebasing before merging; I tested that and it seems to work correctly, in any case.

Also, added a minor comment below.

*/
import debug from './debug';

// For better load performance, these routes are exluded from loading ads.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/exluded/excluded/

@sarayourfriend sarayourfriend force-pushed the update/remove-problematic-language-whitelist-blacklist-analytics branch from c608902 to 9297672 Compare June 29, 2020 17:16
@sarayourfriend
Copy link
Contributor Author

@sgomes Thanks for the review and the heads up. I've rebased and corrected the misspelling 😊

@sarayourfriend sarayourfriend merged commit d5bebcd into master Jun 29, 2020
@sarayourfriend sarayourfriend deleted the update/remove-problematic-language-whitelist-blacklist-analytics branch June 29, 2020 17:23
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 29, 2020
sarayourfriend added a commit that referenced this pull request Jun 29, 2020
sarayourfriend added a commit that referenced this pull request Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Tracks Metrics & Monitoring Capturing analytics about user behavior on WordPress.com.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants