Skip to content

Widget: Added option to set DoNotTrack in Twitter Timeline Widget#9359

Merged
dereksmart merged 5 commits intoAutomattic:masterfrom
stevenlinx:i6722
Jan 2, 2019
Merged

Widget: Added option to set DoNotTrack in Twitter Timeline Widget#9359
dereksmart merged 5 commits intoAutomattic:masterfrom
stevenlinx:i6722

Conversation

@stevenlinx
Copy link
Copy Markdown
Contributor

@stevenlinx stevenlinx commented Apr 19, 2018 •
edited
Loading

Fixes #6722

Changes proposed in this Pull Request:

  • Added option to set DoNotTrack in Twitter Timeline Widget

Testing instructions:

1.) Checkout the code on a test WP site.
2.) Make sure Twitter Timeline Widget is displayed in one of your active sidebars.
3.) Load the site and examine the source code to determine if the hyperlinks on Twitter Timeline Widget comes with DNT attribute.
4.) After done testing, revert the code to its previous state.

Proposed changelog entry for your changes:

  • Widget: Added option to set DoNotTrack in Twitter Timeline Widget

@stevenlinx stevenlinx requested a review from a team as a code owner April 19, 2018 14:43
@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Apr 20, 2018
jeherve
jeherve requested changes Apr 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I think it may be best to add the attribute a bit later, and make it filterable so one can choose to turn this back to false if they want to.

We have a list of $data_attribs just below; you could add it there, and set a default false value for that attribute in the form function, with a filter there.

What do you think?

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Apr 20, 2018
@stevenlinx
Copy link
Copy Markdown
Contributor Author

stevenlinx commented Apr 20, 2018 •
edited
Loading

@jeherve,

I've also thought about this initially and have noticed the $defaults = array( line inside the form function, but didn't do it because I thought $instance array has fixed index parameters from another source.

As a result, I chose to go with what the original ticket issue suggested.

If you're certain this can work, then I agree setting a default is better than hard coding.

I've made the modification.

jeherve
jeherve requested changes Apr 23, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I would recommend adding a filter to offer the option to change the default value via code.

For this reason, when the form is being updated in the update function, I would recommend making sure the value saved for dnt is always a boolean.

modules/widgets/twitter-timeline.php Outdated
'theme' => 'light',
'chrome' => array(),
'tweet-limit' => null,
'dnt' => true,
Copy link
Copy Markdown
Member

@jeherve jeherve Apr 23, 2018

Choose a reason for hiding this comment

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

Could you add a filter here, so folks can choose to change that value to false later on?

@stevenlinx
Copy link
Copy Markdown
Contributor Author

stevenlinx commented Apr 23, 2018 •
edited
Loading

@jeherve

1.) I've added the filter in form() and the type & value checks in update()

2.) I've also revised the value type passed to apply_filters() to string. I think the value type in 'dnt' => apply_filters( 'jetpack_twitter_timeline_default_dnt', 'true' ) should be string based 'true' and 'false' (with single quotes) as opposed to boolean based true and false. Otherwise, it will run into problems with the echo ' data-' on line 140 in widget() func. In other words, if the value type remains a bool, the echo ' data-' line will output "1" instead. In similar fashion, the width and length parameters on 291 and 292 in form() are numbers but its type as defined is string.

The full file can be viewed here:
https://github.com/stevenlin-x/jetpack/blob/7a93bc03a719a4a14e55d3daa100a656c6f3f2a9/modules/widgets/twitter-timeline.php

@stevenlinx stevenlinx changed the title Widget: Added option to set DoNotTrack in Twitter Widget Widget: Added option to set DoNotTrack in Twitter Timeline Widget Jul 4, 2018
@stevenlinx
Copy link
Copy Markdown
Contributor Author

stevenlinx commented Jul 26, 2018

I've made a reply above.

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Sep 5, 2018
jeherve added a commit that referenced this pull request Sep 7, 2018
@jeherve
Twitter Timeline: add DNT option via filter.
2e0a582 
See #9359
@jeherve jeherve mentioned this pull request Sep 7, 2018
Closed
jeherve
jeherve previously requested changes Sep 7, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

After thinking about this some more, I think we could take a simpler approach to this. I created a PR on your repo here with my ideas:
stevenlinx#1

If you like my approach, you can merge that PR and the changes will automatically be ported to this PR.

Let me know what you think.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Sep 7, 2018
@stale
Copy link
Copy Markdown

stale bot commented Dec 6, 2018

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Dec 6, 2018
@stale stale bot removed the [Status] Stale label Dec 6, 2018
@stevenlinx stevenlinx requested a review from a team December 6, 2018 14:00
@jeherve jeherve dismissed their stale review December 6, 2018 14:01

I've pushed the changes to the branch directly.

@jeherve jeherve added this to the 6.9 milestone Dec 6, 2018
@jeherve jeherve self-assigned this Dec 6, 2018
@jeherve jeherve added [Status] Needs Review This PR is ready for review. [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Review This PR is ready for review. labels Dec 6, 2018
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Dec 6, 2018

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS

jeherve
jeherve approved these changes Dec 6, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This should now be good to merge! 👍

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Dec 6, 2018

D21917-code. (newly created revision)

@dereksmart dereksmart merged commit 9f52808 into Automattic:master Jan 2, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 2, 2019
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Changelog: add #9359
d91e0a8
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Jetpack 6.9: Changelog and testing list (#10722)
8d91603 
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@jeherve jeherve

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets Touches WP.com Files

Projects

None yet

Milestone

6.9

Development

Successfully merging this pull request may close these issues.

Twitter Timeline Widget: Add option to set DoNotTrack

5 participants

@stevenlinx @jetpackbot @matticbot @jeherve @dereksmart