Widget: Added option to set DoNotTrack in Twitter Timeline Widget

[stevenlinx]

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

[jeherve]

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?

[stevenlinx]

@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]

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.

[jeherve]

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

[stevenlinx]

@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]

I've made a reply above.

[jeherve]

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.

[stale]

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.

I've pushed the changes to the branch directly.

[jetpackbot]

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]

This should now be good to merge! 👍

[matticbot]

D21917-code. (newly created revision)