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

Issue #841: Native AMP audio and video playlists. #954

Merged
merged 20 commits into from
Feb 18, 2018
Merged

Issue #841: Native AMP audio and video playlists. #954

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
00a3f08
Issue #841: Native AMP video playlists.
Feb 11, 2018
8140a40
Issue #841: Remove dependence on test files.
Feb 11, 2018
cddce46
Issue #841: Add audio playlist shortcode support.
Feb 13, 2018
f19c281
Issue #841: Align @param descriptions in PHP DocBlock.
Feb 13, 2018
ca78258
Issue #841: Enqueue Core playlist styling, and custom styling.
Feb 14, 2018
78f7368
Issue #841: Use wp_json_encode in 'on' attribute.
Feb 15, 2018
ccc066d
Issue #841: Move ternaary conditionals inside escaping functions.
Feb 15, 2018
aba8cf1
Issue #841: Empty string return in audio_playlist().
Feb 15, 2018
f13d2ad
Issue #841: Improve the PLAYLIST_REGEX.
Feb 15, 2018
d3cc386
Issue #841: Make remove_embed() add previous shortcode.
Feb 15, 2018
1b07e90
Issue #841: Return an array() inside the conditional.
Feb 15, 2018
168b9ba
Issue #841: Remove isset() check for $track['src'].
Feb 15, 2018
7cae35c
Issue #841: Set a default height and width for 'audio.'
Feb 15, 2018
d01e9ea
Issue #841: Remove the carousel buttons fro 'audio' playlist.
Feb 15, 2018
e08bc8b
Issue #841: Align equals signs to prevent Travis error.
Feb 15, 2018
b093c85
Enqueue playlist styles just-in-time when used
westonruter Feb 17, 2018
b9d9f09
Prevent playlist scripts from being enqueued which will be stripped o…
westonruter Feb 17, 2018
365af92
Eliminate use of data member var in favor of passing as function arg
westonruter Feb 17, 2018
d94888d
Fix AMP warnings regarding initial states and values
westonruter Feb 18, 2018
13370ed
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter Feb 18, 2018
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
15 changes: 15 additions & 0 deletions assets/css/amp-playlist-shortcode.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* For the custom AMP implementation of audio 'playlist' shortcode.
*/
.wp-playlist .wp-playlist-current-item img {
margin-right: 0;
}

.wp-playlist .wp-playlist-current-item amp-img {
float: left;
margin-right: 10px;
}

.wp-playlist audio {
display: block;
}
32 changes: 30 additions & 2 deletions includes/embeds/class-amp-playlist-embed-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@
*/
class AMP_Playlist_Embed_Handler extends AMP_Base_Embed_Handler {

/**
* The tag of the shortcode.
*
* @var string.
*/
const SHORTCODE = 'playlist';

/**
* The max width of the audio thumbnail image.
*
Expand Down Expand Up @@ -57,7 +64,8 @@ class AMP_Playlist_Embed_Handler extends AMP_Base_Embed_Handler {
* Registers the playlist shortcode.
*/
public function register_embed() {
add_shortcode( 'playlist', array( $this, 'shortcode' ) );
add_shortcode( self::SHORTCODE, array( $this, 'shortcode' ) );
add_action( 'wp_enqueue_scripts', array( $this, 'styling' ) );
Copy link
Contributor Author

@kienstra kienstra Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This uses wp_enqueue_scripts, instead of wp_playlist_script. That enqueues too late.

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed in favor of $this->styling() being called inside of the audio_playlist or video_playlist methods. That should resolve #954 (comment) and it's the right approach anyway, since we only want to enqueue the styles if the shortcode is actually used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @westonruter,
That would definitely be the cleaner appraoch. But it looks like it's not possible to call styling() within AMP_Playlist_Embed_Handler::audio_playlist(), as it's too late to enqueue styling and have AMP_Style_Sanitizer include it in <style amp-custom>. Maybe I'm missing something, though.

}

/**
Expand All @@ -66,7 +74,27 @@ public function register_embed() {
* @return void.
Copy link
Member

Choose a reason for hiding this comment

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

The period shouldn't be used here. A period should only appear after a description when it is provided, for example:

@return int The count.

See examples https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this commit removes that extra period.

*/
public function unregister_embed() {
remove_shortcode( 'playlist' );
remove_shortcode( self::SHORTCODE );
Copy link
Member

Choose a reason for hiding this comment

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

I think the other examples of embeds in the plugin aren't quite right in how they call remove_shortcode(). This should actually be restoring the previous shortcode that was added. So in the register_embed method instead of:

add_shortcode( self::SHORTCODE, array( $this, 'shortcode' ) );

It should do:

global $shortcode_tags;
if ( shortcode_exists( self::SHORTCODE ) ) {
    $this->removed_shortcode = $shortcode_tags[ self::SHORTCODE ];
}
add_shortcode( self::SHORTCODE, array( $this, 'shortcode' ) );

And then in unregister_embed it should do:

add_shortcode( self::SHORTCODE, $this->removed_shortcode );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this commit applies your suggestion of restoring the shortcode before register_embed() changed it.

}

/**
* Enqueues the playlist styling.
*
* @return void.
*/
public function styling() {
global $post;
if ( ! isset( $post->post_content ) || ! has_shortcode( $post->post_content, self::SHORTCODE ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This conditional can be removed if styling is called from inside the playlist method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @westonruter,
It'd be good to remove this conditional. I'll do this if I can figure out the issue that we're discussing above.

return;
}

wp_enqueue_style( 'wp-mediaelement' );
wp_enqueue_style(
'amp-playlist-shortcode',
amp_get_asset_url( 'css/amp-playlist-shortcode.css' ),
array(),
AMP__VERSION
);
}

/**
Expand Down
27 changes: 27 additions & 0 deletions tests/test-class-amp-playlist-embed-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public function test_register_embed() {
$this->instance->register_embed();
$this->assertEquals( 'AMP_Playlist_Embed_Handler', get_class( $shortcode_tags[ $shortcode ][0] ) );
$this->assertEquals( 'shortcode', $shortcode_tags[ $shortcode ][1] );
$this->assertEquals( 10, has_action( 'wp_enqueue_scripts', array( $this->instance, 'styling' ) ) );
$this->instance->unregister_embed();
}

Expand All @@ -78,6 +79,32 @@ public function test_unregister_embed() {
$this->assertFalse( isset( $shortcode_tags[ $shortcode ] ) );
}

/**
* Test styling.
*
* @covers AMP_Playlist_Embed_Handler::styling()
*/
public function test_styling() {
global $post;
$playlist_shortcode = 'amp-playlist-shortcode';
$this->instance->register_embed();
$this->instance->styling();
$styles = wp_styles();
$this->assertFalse( in_array( 'wp-mediaelement', $styles->queue, true ) );
$this->assertFalse( in_array( $playlist_shortcode, $styles->queue, true ) );

$post = $this->factory()->post->create_and_get(); // WPCS: global override OK.
$post->post_content = '[playlist ids="5,3"]';
$this->instance->styling();
$style = $styles->registered[ $playlist_shortcode ];
$this->assertTrue( in_array( 'wp-mediaelement', $styles->queue, true ) );
$this->assertTrue( in_array( $playlist_shortcode, $styles->queue, true ) );
$this->assertEquals( array(), $style->deps );
$this->assertEquals( $playlist_shortcode, $style->handle );
$this->assertEquals( amp_get_asset_url( 'css/amp-playlist-shortcode.css' ), $style->src );
$this->assertEquals( AMP__VERSION, $style->ver );
}

/**
* Test shortcode.
*
Expand Down