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

Added tranformation warning indicator column #898

Conversation

Kluny
Copy link
Contributor

@Kluny Kluny commented Mar 9, 2018

This PR adds a column to wp_edit pages called FBIA Status. The column shows a green light if the post was transformed into a Facebook Instant article with no problems, a yellow light if there were warnings, and a red light if the post was empty after transformation.

There is a checkbox in the FBIA Options page that defaults to false, so that editors can decide whether to show the column or not. The checkbox is placed alongside the "Publish with warnings" option for good grouping of concerns.

A new stylesheet has been added to contain the styling rules for the warning indicators. Since the indicators are very minimal, just red, green, and yellow lights, an informative title text is available on mouseover to help with accessibility.

Relates to #871

Kluny added 4 commits Mar 9, 2018
This branch adds a column to wp_edit pages called FBIA Status. The column shows a green light if the post was transformed into a Facebook Instant article with no problems, a yellow light if there were warnings, and a red light if the post was empty after trasnformation.

There is a checkbox in the FBIA Options page that defaults to false, so that editors can decide whether to show the column or not. The checkbox is placed alongside the "Publish with warnings" option for good grouping of concerns.

A new stylesheet has been added to contain the styling rules for the warning indicators. Since the indicators are very minimal, just red, green, and yellow lights, an informative title text is available on mouseover to help with accessibility.
@Kluny
Copy link
Contributor Author

Kluny commented Mar 9, 2018

Here's how the column looks in index view.

image

Here's the option in FBIA settings.

image

@adamsilverstein
Copy link

adamsilverstein commented Mar 9, 2018

Nice work @Kluny this seems like a super useful feature!

}
return $columns;
}
add_filter('manage_posts_columns', 'fbia_indicator_column_heading');

Choose a reason for hiding this comment

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

missing some whitespace here: add_filter( 'manage_posts_columns', 'fbia_indicator_column_heading' );

$post = get_post( $post_ID );
$instant_articles_post = new \Instant_Articles_Post( $post );

$is_empty = esc_html( $instant_articles_post->is_empty_after_transformation() );

Choose a reason for hiding this comment

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

I don't think you need esc_html here, it is for output


$is_empty = esc_html( $instant_articles_post->is_empty_after_transformation() );
if ( "1" === $is_empty ) {
echo $red_light;

Choose a reason for hiding this comment

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

add wp_kses_post: echo wp_kses_post( $red_light );

return;
}

$has_warnings = esc_html( $instant_articles_post->has_warnings_after_transformation() );

Choose a reason for hiding this comment

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

remove esc_html


$has_warnings = esc_html( $instant_articles_post->has_warnings_after_transformation() );
if ( "1" === $has_warnings ) {
echo $yellow_light;

Choose a reason for hiding this comment

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

add wp_kses_post

return;
}

echo $green_light;

Choose a reason for hiding this comment

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

add wp_kses_post

}
}
}
add_action('manage_posts_custom_column', 'fbia_indication_column', 10, 2);

Choose a reason for hiding this comment

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

add whitespace

$display_warning_column = $publishing_settings[ 'display_warning_column' ];

if( "1" === $display_warning_column ) {
$red_light = "<span title='Instant article is empty after transformation.' class='instant-articles-col-status error.'></span>";

Choose a reason for hiding this comment

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

In these html strings, the double quotes should be part of the html and the single quotes in the code, unless i'm missing something... so for this one:
$red_light = '';

Kluny added 2 commits Mar 9, 2018
Removed escaping where unneeded, and whitespace fixes.
@Kluny
Copy link
Contributor Author

Kluny commented Mar 9, 2018

Made changes as suggested.

Copy link
Collaborator

@everton-rosario everton-rosario left a comment

I loved this PR!
Really amazing work here @Kluny
Sorry for the long wait on reviewing it.

I'm just requesting small changes to make it more clear on label/title of strings.

Also double check the if checks to properly display colors. On my env tests it was all painted green. Check my inline comments for the update.

Again, nice work!
Thanks for initiative and the PR.

$display_warning_column = $publishing_settings[ 'display_warning_column' ];

if( "1" === $display_warning_column ) {
$columns[ 'FBIA' ] = "<span title='Facebook Instant Article Distribution Status' class='fbia-col-heading'>FBIA Status</span>";
Copy link
Collaborator

@everton-rosario everton-rosario Mar 23, 2018

Choose a reason for hiding this comment

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

Can you change the title here to FB IA Status?
$columns[ 'FBIA' ] = "<span title='Facebook Instant Article Distribution Status' class='fbia-col-heading'>FB IA Status</span>";

$instant_articles_post = new \Instant_Articles_Post( $post );

$is_empty = $instant_articles_post->is_empty_after_transformation();
if ( "1" === $is_empty ) {
Copy link
Collaborator

@everton-rosario everton-rosario Mar 23, 2018

Choose a reason for hiding this comment

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

Given the code from is_empty_after_transformation() function never returning something different from a bool, it would be enough for you to use the test as:
if ( true === $is_empty ) {

The test with strings are not working properly into all the versions.

}

$has_warnings = $instant_articles_post->has_warnings_after_transformation();
if ( "1" === $has_warnings ) {
Copy link
Collaborator

@everton-rosario everton-rosario Mar 23, 2018

Choose a reason for hiding this comment

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

Given the code from has_warnings_after_transformation() function never returning something different from a bool, it would be enough for you to use the test as:
if ( true === $has_warnings ) {

The test with strings are not working properly into all the versions.

@@ -55,6 +55,14 @@ class Instant_Articles_Option_Publishing extends Instant_Articles_Option {
'checkbox_label' => 'Publish articles containing warnings',
),

'display_warning_column' => array(
'label' => 'Transformation warning column',
Copy link
Collaborator

@everton-rosario everton-rosario Mar 23, 2018

Choose a reason for hiding this comment

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

Using title exactly the same from the listing, looks better:

'label' => 'FB IA Status column',

'description' => 'With this option enabled, a column will be added to post indexes to quickly see whether an article transformation failed, succeeded, or had warnings.',
'render' => 'checkbox',
'default' => false,
'checkbox_label' => 'Show column',
Copy link
Collaborator

@everton-rosario everton-rosario Mar 23, 2018

Choose a reason for hiding this comment

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

Changing to properly explain what happens on selecting this checkbox
'checkbox_label' => 'Enable column "FB IA Status"',

- Changed column title
- Changed setting label to match
- Changed if statements to check for true instead of string "1".
@Kluny
Copy link
Contributor Author

Kluny commented Mar 23, 2018

Added fixes as suggested.

Also double check the if checks to properly display colors. On my env tests it was all painted green.

@everton-rosario, here's some HTML that should cause a warning yellow light - use the classic editor in HTML text mode, enter this string and update the post.

<div class="media-credit-container alignnone"> Some stuff in here </div>

Thanks for the CR!

Copy link
Collaborator

@diegoquinteiro diegoquinteiro left a comment

Thank you so much for putting this together. It looks perfect to me!

@Kluny
Copy link
Contributor Author

Kluny commented Mar 28, 2018

@vkama, I think you have the ability to merge branches - can this one be merged?

@Kluny
Copy link
Contributor Author

Kluny commented Apr 6, 2018

@mburak, just wondering if this branch can get merged?

@adamsilverstein
Copy link

adamsilverstein commented May 9, 2018

@everton-rosario has your feedback been addressed here? can this be merged?

@adamsilverstein
Copy link

adamsilverstein commented Jun 4, 2018

@everton-rosario @mburak Any additional changes you would like to see here to get this in?

Can we get this work merged before it becomes stale?

@everton-rosario everton-rosario merged commit 8fdf42f into Automattic:master Jun 4, 2018
@everton-rosario
Copy link
Collaborator

everton-rosario commented Jun 4, 2018

Sorry for the delay @adamsilverstein

@Kluny
Copy link
Contributor Author

Kluny commented Jun 4, 2018

Thank you @everton-rosario!

@adamsilverstein
Copy link

adamsilverstein commented Jun 5, 2018

Woo hoo! Thanks @everton-rosario.

@everton-rosario
Copy link
Collaborator

everton-rosario commented Jun 6, 2018

This will be out on the next release. Till there, you can get it from master ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants