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

Add AMP support for the Twitter Timeline widget #15328

Merged
merged 1 commit into from
Apr 24, 2020
Merged

Add AMP support for the Twitter Timeline widget #15328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 54 additions & 42 deletions modules/widgets/twitter-timeline.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ public function __construct() {
* Enqueue scripts.
*/
public function enqueue_scripts() {
wp_enqueue_script( 'jetpack-twitter-timeline' );
if ( ! class_exists( 'Jetpack_AMP_Support' ) || ! Jetpack_AMP_Support::is_amp_request() ) {
wp_enqueue_script( 'jetpack-twitter-timeline' );
}
}

/**
Expand Down Expand Up @@ -84,59 +86,54 @@ public function admin_scripts( $hook ) {
* @param array $instance Saved values from database.
*/
public function widget( $args, $instance ) {
$output = '';

// Twitter deprecated `data-widget-id` on 2018-05-25,
// with cease support deadline on 2018-07-27.
if ( isset( $instance['type'] ) && 'widget-id' === $instance['type'] ) {
if ( current_user_can( 'edit_theme_options' ) ) {
echo $args['before_widget'];
echo $args['before_title'] . esc_html__( 'Twitter Timeline', 'jetpack' ) . $args['after_title'];
echo '<p>' . esc_html__( "The Twitter Timeline widget can't display tweets based on searches or hashtags. To display a simple list of tweets instead, change the Widget ID to a Twitter username. Otherwise, delete this widget.", 'jetpack' ) . '</p>';
echo '<p>' . esc_html__( '(Only administrators will see this message.)', 'jetpack' ) . '</p>';
echo $args['after_widget'];
$output .= $args['before_widget']
. $args['before_title'] . esc_html__( 'Twitter Timeline', 'jetpack' ) . $args['after_title']
. '<p>' . esc_html__( "The Twitter Timeline widget can't display tweets based on searches or hashtags. To display a simple list of tweets instead, change the Widget ID to a Twitter username. Otherwise, delete this widget.", 'jetpack' ) . '</p>'
. '<p>' . esc_html__( '(Only administrators will see this message.)', 'jetpack' ) . '</p>'
. $args['after_widget'];
}

echo $output; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
return;
}

$instance['lang'] = substr( strtoupper( get_locale() ), 0, 2 );

echo $args['before_widget'];
$output .= $args['before_widget'];

$title = isset( $instance['title'] ) ? $instance['title'] : '';

/** This filter is documented in core/src/wp-includes/default-widgets.php */
$title = apply_filters( 'widget_title', $title );
if ( ! empty( $title ) ) {
echo $args['before_title'] . $title . $args['after_title'];
}

if ( isset( $instance['type'] ) && 'widget-id' === $instance['type'] && current_user_can( 'edit_theme_options' ) ) {
kraftbj marked this conversation as resolved.
Show resolved Hide resolved
echo '<p>' . esc_html__( 'As of July 27, 2018, the Twitter Timeline widget will no longer display tweets based on searches or hashtags. To display a simple list of tweets instead, change the Widget ID to a Twitter username.', 'jetpack' ) . '</p>';
echo '<p>' . esc_html__( '(Only administrators will see this message.)', 'jetpack' ) . '</p>';
$output .= $args['before_title'] . $title . $args['after_title'];
}

// Start tag output
// This tag is transformed into the widget markup by Twitter's
// widgets.js code
echo '<a class="twitter-timeline"';

$data_attribs = array(
$possible_data_attribs = array(
'width',
'height',
'theme',
'border-color',
'tweet-limit',
'lang',
);
foreach ( $data_attribs as $att ) {
$data_attrs = '';
foreach ( $possible_data_attribs as $att ) {
if ( ! empty( $instance[ $att ] ) && ! is_array( $instance[ $att ] ) ) {
echo ' data-' . esc_attr( $att ) . '="' . esc_attr( $instance[ $att ] ) . '"';
$data_attrs .= ' data-' . esc_attr( $att ) . '="' . esc_attr( $instance[ $att ] ) . '"';
}
}

/** This filter is documented in modules/shortcodes/tweet.php */
$partner = apply_filters( 'jetpack_twitter_partner_id', 'jetpack' );
if ( ! empty( $partner ) ) {
echo ' data-partner="' . esc_attr( $partner ) . '"';
$data_attrs .= ' data-partner="' . esc_attr( $partner ) . '"';
}

/**
Expand All @@ -152,29 +149,13 @@ public function widget( $args, $instance ) {
*/
$dnt = apply_filters( 'jetpack_twitter_timeline_default_dnt', false );
if ( true === $dnt ) {
echo ' data-dnt="true"';
$data_attrs .= ' data-dnt="true"';
}

if ( ! empty( $instance['chrome'] ) && is_array( $instance['chrome'] ) ) {
echo ' data-chrome="' . esc_attr( join( ' ', $instance['chrome'] ) ) . '"';
$data_attrs .= ' data-chrome="' . esc_attr( join( ' ', $instance['chrome'] ) ) . '"';
}

$type = ( isset( $instance['type'] ) ? $instance['type'] : '' );
$widget_id = ( isset( $instance['widget-id'] ) ? $instance['widget-id'] : '' );
switch ( $type ) {
case 'profile':
echo ' href="https://twitter.com/' . esc_attr( $widget_id ) . '"';
break;
case 'widget-id':
default:
echo ' data-widget-id="' . esc_attr( $widget_id ) . '"';
break;
}
echo ' href="https://twitter.com/' . esc_attr( $widget_id ) . '"';
Comment on lines -162 to -173
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much of this PR's diff is simply moving logic to different lines, and changing echo to $output .=.

For example, this snippet is simply moved lower in the file, with echo changed to $output .=


// End tag output
echo '>';

$timeline_placeholder = __( 'My Tweets', 'jetpack' );

/**
Expand All @@ -188,11 +169,42 @@ public function widget( $args, $instance ) {
*/
$timeline_placeholder = apply_filters( 'jetpack_twitter_timeline_placeholder', $timeline_placeholder );

echo esc_html( $timeline_placeholder ) . '</a>';
$type = ( isset( $instance['type'] ) ? $instance['type'] : '' );
$widget_id = ( isset( $instance['widget-id'] ) ? $instance['widget-id'] : '' );

if ( class_exists( 'Jetpack_AMP_Support' ) && Jetpack_AMP_Support::is_amp_request() ) {
$width = ! empty( $instance['width'] ) ? $instance['width'] : 600;
$height = ! empty( $instance['height'] ) ? $instance['height'] : 480;
$output .= '<amp-twitter' . $data_attrs . ' layout="responsive" data-timeline-source-type="profile" data-timeline-screen-name="' . esc_attr( $widget_id ) . '" width="' . absint( $width ) . '" height="' . absint( $height ) . '">'; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
$output .= esc_html( $timeline_placeholder ) . '</amp-twitter>';

echo $output . $args['after_widget']; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
return;
}
Comment on lines +175 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is currently needed because the AMP plugin has a bug in how it recognizes markup for Twitter embeds. Namely, at the moment it is only recognizing div elements with the twitter-tweet class name:

https://github.com/ampproject/amp-wp/blob/4880f0f58daaf07685854be8574ff25d76ff583e/includes/embeds/class-amp-twitter-embed-handler.php#L146

It should be expanded to also recognize twitter-timeline as well. If so, then I believe this condition can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've filed an issue to fix this in the AMP plugin: ampproject/amp-wp#4653

With that done, it may make sense to remove this condition as it won't strictly be needed anymore. That being said, it is not bad to output <amp-twitter> markup from the start! It can be somewhat preferable to relying on the AMP plugin do the conversion.


// Start tag output
// This tag is transformed into the widget markup by Twitter's
// widgets.js code.
$output .= '<a class="twitter-timeline"' . $data_attrs; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
switch ( $type ) {
case 'profile':
$output .= ' href="https://twitter.com/' . esc_attr( $widget_id ) . '"';
break;
case 'widget-id':
default:
$output .= ' data-widget-id="' . esc_attr( $widget_id ) . '"';
break;
}
$output .= ' href="https://twitter.com/' . esc_attr( $widget_id ) . '"';

// End tag output.
$output .= '>';

$output .= esc_html( $timeline_placeholder ) . '</a>';

// End tag output

echo $args['after_widget'];
echo $output . $args['after_widget']; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped

/** This action is documented in modules/widgets/gravatar-profile.php */
do_action( 'jetpack_stats_extra', 'widget_view', 'twitter_timeline' );
Expand Down
135 changes: 135 additions & 0 deletions tests/php/modules/widgets/test_twitter-timeline-widget.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
<?php // phpcs:ignore Wordpress.Files.FileName.NotHyphenatedLowercase
/**
* Test Jetpack_Twitter_Timeline_Widget.
*
* @package Jetpack
*/

require dirname( __FILE__ ) . '/../../../../modules/widgets/twitter-timeline.php';

/**
* Test Jetpack_Twitter_Timeline_Widget.
*/
class WP_Test_Twitter_Timeline_Widget extends WP_UnitTestCase {

/**
* The tested instance.
*
* @var Jetpack_Twitter_Timeline_Widget
*/
public $instance;

/**
* Sets up each test.
*
* @inheritDoc
*/
public function setUp() {
parent::setUp();
$this->instance = new Jetpack_Twitter_Timeline_Widget();
}

/**
* Gets the test data for test_widget().
*
* @return array The test data.
*/
public function get_widget_data() {
return array(
'no_id' => array(
array(),
false,
'<div><a class="twitter-timeline" data-lang="EN" data-partner="jetpack" data-widget-id="" href="https://twitter.com/">My Tweets</a></div>',
),
'type_is_widget_id' => array(
array( 'type' => 'widget-id' ),
false,
'<div><h1>Twitter Timeline</h1><p>The Twitter Timeline widget can&#039;t display tweets based on searches or hashtags. To display a simple list of tweets instead, change the Widget ID to a Twitter username. Otherwise, delete this widget.</p><p>(Only administrators will see this message.)</p></div>',
),
'only_widget_id_present' => array(
array( 'widget-id' => 'wordpress' ),
false,
'<div><a class="twitter-timeline" data-lang="EN" data-partner="jetpack" data-widget-id="wordpress" href="https://twitter.com/wordpress">My Tweets</a></div>',
),
'type_is_profile' => array(
array(
'widget-id' => 'wordpress',
'type' => 'profile',
),
false,
'<div><a class="twitter-timeline" data-lang="EN" data-partner="jetpack" href="https://twitter.com/wordpress" href="https://twitter.com/wordpress">My Tweets</a></div>',
),
'with_data_attributes' => array(
array(
'width' => '200',
'height' => '400',
'theme' => 'dark',
'border-color' => '#ffffff',
'tweet-limit' => '9',
'lang' => 'es',
),
false,
'<div><a class="twitter-timeline" data-width="200" data-height="400" data-theme="dark" data-border-color="#ffffff" data-tweet-limit="9" data-lang="EN" data-partner="jetpack" data-widget-id="" href="https://twitter.com/">My Tweets</a></div>',
),
'data_chrome' => array(
array(
'widget-id' => 'wordpress',
'chrome' => array( 'noborders', 'nofooter' ),
),
false,
'<div><a class="twitter-timeline" data-lang="EN" data-partner="jetpack" data-chrome="noborders nofooter" data-widget-id="wordpress" href="https://twitter.com/wordpress">My Tweets</a></div>',
),
'amp_no_widget_id_present' => array(
array(),
true,
'<div><amp-twitter data-lang="EN" data-partner="jetpack" layout="responsive" data-timeline-source-type="profile" data-timeline-screen-name="" width="600" height="480">My Tweets</amp-twitter></div>',
),
'amp_widget_id_present' => array(
array( 'widget-id' => 'wordpress' ),
true,
'<div><amp-twitter data-lang="EN" data-partner="jetpack" layout="responsive" data-timeline-source-type="profile" data-timeline-screen-name="wordpress" width="600" height="480">My Tweets</amp-twitter></div>',
),
'amp_with_data_attributes' => array(
array(
'width' => '200',
'height' => '800',
'theme' => 'light',
'border-color' => '#ff0000',
'tweet-limit' => '4',
'lang' => 'cnr',
),
true,
'<div><amp-twitter data-width="200" data-height="800" data-theme="light" data-border-color="#ff0000" data-tweet-limit="4" data-lang="EN" data-partner="jetpack" layout="responsive" data-timeline-source-type="profile" data-timeline-screen-name="" width="200" height="800">My Tweets</amp-twitter></div>',
),
);
}

/**
* Test the widget method that outputs the markup.
*
* @dataProvider get_widget_data
* @covers Jetpack_Twitter_Timeline_Widget::widget()
*
* @param array $instance The widget instance.
* @param bool $is_amp Whether this is on an AMP endpoint.
* @param string $expected The expected output of the tested method.
*/
public function test_widget( $instance, $is_amp, $expected ) {
wp_set_current_user( $this->factory()->user->create( array( 'role' => 'administrator' ) ) );
if ( $is_amp ) {
add_filter( 'jetpack_is_amp_request', '__return_true' );
}

$args = array(
'before_widget' => '<div>',
'after_widget' => '</div>',
'before_title' => '<h1>',
'after_title' => '</h1>',
);

ob_start();
$this->instance->widget( $args, $instance );

$this->assertEquals( $expected, ob_get_clean() );
}
}