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

Disqus Comment Widget Replacement #2613

Merged
merged 10 commits into from
Jun 23, 2020
Merged

Disqus Comment Widget Replacement #2613

merged 10 commits into from
Jun 23, 2020

Conversation

ablanathtanalba
Copy link
Contributor

This replaces the previous PR #2271 with one that's more up-to-date from master's current state.

@ghostwords

This comment has been minimized.

Sign in to view
@ablanathtanalba

This comment has been minimized.

Sign in to view
@ablanathtanalba ablanathtanalba marked this pull request as ready for review June 2, 2020 15:22
@ablanathtanalba

This comment has been minimized.

Sign in to view
@ghostwords ghostwords added the widgets Click-to-activate placeholders for blocked but potentially useful social buttons/widgets label Jun 4, 2020
ghostwords
ghostwords reviewed Jun 4, 2020
View reviewed changes
src/js/webrequest.js Outdated Show resolved Hide resolved
src/js/webrequest.js Outdated Show resolved Hide resolved
src/data/socialwidgets.json Outdated Show resolved Hide resolved
@ablanathtanalba
Copy link
Contributor Author

ablanathtanalba commented Jun 11, 2020 •
edited
Loading

@ghostwords the latest commit applies the simplification we spoke about in the developer call earlier today. I also added the wildcard support to getWidgetList(), although when I test it on the https://cyphe.rs/test/disqus.html site, I'm still only able to get the widget to replace when I manually set disqus.com to block through the popup. I'm not sure what else I'm missing here -- is it because the disqus domain still exists on the yellowlist?

ghostwords
ghostwords reviewed Jun 15, 2020
View reviewed changes
src/js/webrequest.js Outdated Show resolved Hide resolved
@ablanathtanalba
Copy link
Contributor Author

Rebased with master with the most recent commit. Did some more manual testing to make sure it works elsewhere, on both the standalone test fixture and out in the wild:
https://dirkhoag.com/2012/12/wordpress-disqus-recent-comments-widget-466
https://cyphe.rs/test/disqus.html

@ghostwords
Remove unneeded/redundant Disqus widget domains
e91262b
@ghostwords
Simplify wildcard allowed widget domains
20deb05 
No need for regexes yet.
@ghostwords
Fix nits in replacement domain matching
26a43bc 
- If there are multiple wildcard domain matches,
  make sure all of them were blocked

- If we have a wildcard domain that didn't match our criteria,
  don't keep going onto non-wildcard domain logic
ghostwords
ghostwords approved these changes Jun 22, 2020
View reviewed changes
Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

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

Looks good generally. Thank you!

I'll do some more testing with my followup commits tomorrow.

@ghostwords
Fix wildcard domains for dynamic widget elements
799e4c2 
Domain matching for widget elements created after initial page load
needed to be updated to support wildcards.
@ghostwords
Copy link
Member

ghostwords commented Jun 23, 2020 •
edited
Loading

Dynamically inserted widget elements didn't work with wildcard matching. For example, click "Comments" on this page. Fixed by 799e4c2.

@ghostwords ghostwords merged commit 54937ae into master Jun 23, 2020
@ghostwords ghostwords deleted the disqus-widget-replacement branch June 23, 2020 18:23
ghostwords added a commit that referenced this pull request Jul 14, 2020
@ghostwords
Remove Disqus domains from yellowlist
a002d21 
Following up on #2613
ghostwords added a commit that referenced this pull request Jul 14, 2020
@ghostwords
Add two Disqus domains to MDFP list
1e9f217 
Following up on #2613
ghostwords added a commit that referenced this pull request Jul 16, 2020
@ghostwords
Restore disquscdn.com to yellowlist
1e696b5 
As mitigation until the MDFP list for Disqus is distributed (1e9f217).

Also to help users who cookieblock or allow the *.disqus.com domain, but
don't think to move the slider for disquscdn.com domains.

Following up on #2613
@Centaurioun Centaurioun mentioned this pull request Jan 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghostwords ghostwords ghostwords approved these changes

Assignees
No one assigned
Labels
widgets Click-to-activate placeholders for blocked but potentially useful social buttons/widgets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ablanathtanalba @ghostwords