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

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

merged 1 commit into from
Apr 24, 2020

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Apr 7, 2020

  • Add AMP support to Twitter Timeline widget
  • No intended change to the non-AMP version

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Enhances an existing feature

Testing instructions:

  1. Ensure the AMP plugin is active
  2. In AMP Settings, select Transitional mode
  3. In Jetpack > Settings > Writing, ensure this is toggled on:

twitter-timeline-widget

  1. Go to the Customizer
  2. Add a Twitter Timeline widget
  3. Populate the widget. Here's an example:
    twitter-timeline-widget
  4. Go to the front-end of the AMP URL
  5. Expected: A Twitter timeline displays, like:

twitter-timeline-w

  1. Test changing the widget settings, like changing the theme to 'Dark,' or changing the '# of tweets shown'
  2. Ensure that on non-AMP URLs, this PR has no effect

Proposed changelog entry for your changes:

  • Add AMP support for the Twitter Timeline widget

@kienstra
Add AMP support for the Twitter Timeline widget
6215533 
Use the <amp-twitter> component,
and add unit tests for several cases.
@kienstra kienstra requested a review from a team April 7, 2020 04:47
@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 6215533

kienstra
kienstra commented Apr 7, 2020
View reviewed changes
modules/widgets/twitter-timeline.php
Comment on lines -162 to -173
$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 ) . '"';
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 .=

@kienstra kienstra self-assigned this Apr 7, 2020
@kienstra kienstra added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Apr 7, 2020
@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it AMP [Feature] Extra Sidebar Widgets labels Apr 14, 2020
@jeherve jeherve added this to the 8.5 milestone Apr 14, 2020
@kraftbj kraftbj requested review from kraftbj and removed request for a team April 24, 2020 16:31
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello kienstra! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D42404-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

@kraftbj
Copy link
Contributor

kraftbj commented Apr 24, 2020

r206428-wpcom

@kraftbj kraftbj merged commit bdf1846 into master Apr 24, 2020
@kraftbj kraftbj deleted the update/twitter-timeline-amp branch April 24, 2020 16:48
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 24, 2020
jeherve added a commit that referenced this pull request Apr 27, 2020
@jeherve
Changelog: add #15328
a741f95
westonruter
westonruter reviewed May 4, 2020
View reviewed changes
modules/widgets/twitter-timeline.php
Comment on lines +175 to +183
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;
}
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.

@westonruter westonruter mentioned this pull request May 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter left review comments

@kraftbj kraftbj kraftbj approved these changes

Assignees

@kienstra kienstra

Labels
AMP [Feature] Extra Sidebar Widgets Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.5
Development

Successfully merging this pull request may close these issues.
6 participants
@kienstra @jetpackbot @matticbot @kraftbj @westonruter @jeherve