Add AMP icon if AMP is enabled on single error page #1473
Conversation
|
@jacobschweitzer Could you please add before/after screenshots in PRs like this? |
|
@westonruter Of course. Here we are: Before: After: |
…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() ) { |
There was a problem hiding this comment.
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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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' ); |
There was a problem hiding this comment.
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' ); |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 ) ); | |||
There was a problem hiding this comment.
This can remain just %s because there is only one placeholder.
There was a problem hiding this comment.
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(); |
No description provided.