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

Add AMP icon if AMP is enabled on single error page #1473

Merged
merged 11 commits into from Sep 27, 2018

Conversation

Projects
None yet
2 participants
@jacobschweitzer
Copy link
Contributor

jacobschweitzer commented Sep 27, 2018

No description provided.

@jacobschweitzer jacobschweitzer requested a review from westonruter Sep 27, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Sep 27, 2018

@jacobschweitzer Could you please add before/after screenshots in PRs like this?

@jacobschweitzer

This comment has been minimized.

Copy link
Contributor Author

jacobschweitzer commented Sep 27, 2018

@westonruter Of course. Here we are:

Before:
screen shot 2018-09-27 at 2 18 06 pm

After:
screen shot 2018-09-27 at 2 18 17 pm

jacobschweitzer added some commits Sep 27, 2018

Merge remote-tracking branch 'origin/develop' into add/amp-icon-if-en…
…abled-single-url

# Conflicts:
#	assets/css/amp-validation-single-error-url.css
*
* @return bool|void
*/
public static function is_amp_enabled_on_post( $post, $validation_errors = array(), $counts = array() ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

I think it would be better to eliminate the $validation_errors and $counts params. This simplifies the API. Any previously-obtained terms will already be stored in the object cache, so there is not really much concern about duplicated work.

$counts['ack_rejected']++;
break;
}
}

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

This foreach loop is duplicated with display_invalid_url_validation_error_counts_summary. I suggest adding a protected method for count_invalid_url_validation_errors() which you pass the $validation_errors array both here and in display_invalid_url_validation_error_counts_summary.

component.showAMPIconIfEnabled = function() {
const heading = document.getElementsByClassName( 'wp-heading-inline' );
if ( heading[ 0 ] && true === component.data.l10n.amp_enabled ) {
let ampIcon = document.createElement( 'span' );

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

This should use const not let.

* Adds the AMP icon to the page heading if AMP is enabled on this URL.
*/
component.showAMPIconIfEnabled = function() {
const heading = document.getElementsByClassName( 'wp-heading-inline' );

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

This can use querySelector instead of getElementsByClassName. So:

const heading = document.querySelector( '.wp-heading-inline' );

Then below just check to see if heading is truthy as opposed to heading[0]

return;
}
$post = get_post( intval( $_GET['post'] ) ); // WPCS: CSRF OK.

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

I don't think this is necessary because the $post global will already be populated on the edit post screen. So instead of $post this you can just use get_post() below.

@@ -2007,7 +2008,7 @@ public static function get_single_url_page_heading() {
}
/* translators: %s is the name of the page with the the validation error(s) */
return esc_html( sprintf( __( 'Errors for: %s', 'amp' ), $name ) );
return esc_html( sprintf( __( 'Errors for: %1$s', 'amp' ), $name ) );

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

This can remain just %s because there is only one placeholder.

This comment has been minimized.

Copy link
@jacobschweitzer

jacobschweitzer Sep 27, 2018

Author Contributor

Oh good catch, I tried adding the icon in here before but realized it was added as text via JS so it didn't work that way.

$pagenow = 'post.php';
$pagenow = 'post.php';
$amp_invalid_url_post = $this->factory()->post->create_and_get( array( 'post_type' => AMP_Invalid_URL_Post_Type::POST_TYPE_SLUG ) );
$post = $this->factory()->post->create_and_get();

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

Make this global here.

jacobschweitzer added some commits Sep 27, 2018

@westonruter westonruter added this to the v1.0 milestone Sep 27, 2018

@westonruter westonruter merged commit b83c6fe into develop Sep 27, 2018

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

@westonruter westonruter deleted the add/amp-icon-if-enabled-single-url branch Sep 27, 2018

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.