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.

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

Adding support to IA->AMP conversion #680

Merged
merged 19 commits into from Jun 2, 2017

Conversation

Projects
None yet
3 participants
@vkama
Copy link
Collaborator

vkama commented May 30, 2017

This PR is to enable the IA->AMP conversion from articles. It includes the newly released extensions SDK.

@vkama vkama requested review from everton-rosario and diegoquinteiro May 30, 2017

@vkama vkama added this to the 4.0 milestone May 30, 2017

}
/**
* Helper function to validate the json string

This comment has been minimized.

Copy link
@everton-rosario

everton-rosario Jun 1, 2017

Collaborator

Check whitespacing formats in this block
💅

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

👌

$adapter = new Instant_Articles_Post( $post );
$article = $adapter->to_instant_article();
$article_html = $article->render();
$amp = AMPArticle::create($article_html, $properties);

This comment has been minimized.

Copy link
@everton-rosario

everton-rosario Jun 1, 2017

Collaborator

Lets separate this AMP generation into a filter:
"get_amp_content"

Then the die call should be from an action

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

same as above comment

* NOTE: side-effect: function calls die() in the end.
* @since 4.0
*/
static function markup_version( ) {

This comment has been minimized.

Copy link
@everton-rosario

everton-rosario Jun 1, 2017

Collaborator

Make it available to create filter and embed the filter generation into a separated action, this way we can allow extension for this great block.

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

Let's call this method render_amp.

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

@everton-rosario Yeah, I will do that in a following PR like we discussed

@diegoquinteiro naming was done to be in sync with the ia_markup_version(), we can change both in a following PR

@diegoquinteiro
Copy link
Collaborator

diegoquinteiro left a comment

That's a great implementation. In fact, several of these ideas should make their way to the OG tag generation.

There are some minor nits to fix/discuss.

self::$settings = Instant_Articles_Option_Amp::get_option_decoded();
return self::$settings;
}

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

💅 add an empty line between the methods

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

👌

* @since 4.0
*/
static function get_settings() {
if (self::$settings === null)

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

💅 some code standard nits:

  • always use the full if notation with brackets;
  • add padding to parenthesis ( self::$settings === null )

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

done

*/
static function inject_link_rel() {
if (!self::is_markup_enabled())

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

💅

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

done

// Transform the post to an Instant Article.
$adapter = new Instant_Articles_Post( get_post() );
if (!$adapter->should_submit_post())

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

💅

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

Let's create a new method should_generate_amp() somewhere and rename this one to should_generate_ia(). There will be situations where the article will be generated as IA but not as AMP, such as when the option for generating AMP is disabled.

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

as we discussed offline, we will do it in a separate adapter for the amp stuff

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

and btw, the latter would not happen since I check for is_markup_enabled()

* NOTE: side-effect: function calls die() in the end.
* @since 4.0
*/
static function markup_version( ) {

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

Let's call this method render_amp.

),
Instant_Articles_Amp_Markup::SETTING_DL_MEDIA => array(
'label' => 'Image sizing',

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

"Automatic image sizing"

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

better wording, done.

'label' => 'Image sizing',
'description' => 'With this option enabled, images in other servers/domains will be downloaded to get it\'s width and height. Learn more in <a href="https://developers.facebook.com/docs/instant-articles/other-formats#https://developers.facebook.com/docs/instant-articles/other-formats#advanced-media-element-sizing" target="_blank"> the official docs</a>',
'render' => 'checkbox',
'checkbox_label' => 'Download Images hosted elsewhere',

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

I'd go with something like: "Download all external images to get their dimensions (slow)"

We want users to be careful with this option.

Instant_Articles_Amp_Markup::SETTING_DL_MEDIA => array(
'label' => 'Image sizing',
'description' => 'With this option enabled, images in other servers/domains will be downloaded to get it\'s width and height. Learn more in <a href="https://developers.facebook.com/docs/instant-articles/other-formats#https://developers.facebook.com/docs/instant-articles/other-formats#advanced-media-element-sizing" target="_blank"> the official docs</a>',

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

Wouldn't it be "(...) to get their width and height (...)"

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

oops

@@ -37,6 +37,7 @@ class Instant_Articles_Option_FB_Page extends Instant_Articles_Option {
*
* @since 0.4
*/
//TODO: this seems to be bugged, please check (vk)

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

🤡

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

really, this is bugged! need to address that in a separated PR

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 2, 2017

Collaborator

Please remove the TODO and create an issue on milestone 4.0.

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

issue opened:

#683

use Facebook\InstantArticles\AMP\AMPArticle;
use Facebook\InstantArticles\Validators\Type;
class Instant_Articles_Amp_Markup {

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 1, 2017

Collaborator

Let's make it Instant_Articles_AMP_Markup?

This comment has been minimized.

Copy link
@vkama

vkama Jun 2, 2017

Author Collaborator

done. also renamed the options class to _AMP

vkama added some commits Jun 2, 2017

Added wp-coding-standards/wpcs as a dev requirement. Fixed all the ni…
…ts with phpcbf. Additional stuff as per reviews.
@diegoquinteiro

This comment has been minimized.

Copy link
Collaborator

diegoquinteiro commented Jun 2, 2017

I've approved the changes, but please remove the TODO before merging.

@vkama vkama merged commit ec930fa into v4.0 Jun 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@vkama vkama deleted the amp branch Jun 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.