-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Try applying notice classes with JS templates #5225
Conversation
- Uses echo wp_get_admin_notice() because wp_kses_post removes template conditionals - Adds function to output data because there are two usages in theme-install.php and the only difference is the notice class. - Function will be easier to clone for use in other similar contexts.
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
…press-develop into 57791-jstemplates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good @joedolson! I hadn't gotten a chance to follow up on this so thanks for continuing work here!
Just left a few queries, and some alignment corrections.
* @type string $requires_php The version of PHP which the plugin requires. | ||
* } | ||
*/ | ||
$compatibility_notice . do_action( "in_plugin_update_message-{$file}", $plugin_data, $response ), // phpcs:ignore WordPress.NamingConventions.ValidHookName.UseUnderscores, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since do_action()
returns void
, this appends null
(or ''
) to the notice. While this doesn't present an issue for the message, I wonder if we should just call do_action()
above the wp_admin_notice()
call?
* @type string $package Theme update package URL. | ||
* } | ||
*/ | ||
$compatibility_notice . do_action( "in_theme_update_message-{$theme_key}", $theme, $response ), // phpcs:ignore WordPress.NamingConventions.ValidHookName.UseUnderscores |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same query as above
</p></div> | ||
<?php | ||
$compatibility_template = theme_compatibility_template(); | ||
echo wp_get_admin_notice( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used instead of wp_admin_notice()
because of escaping issues? If so, we might consider adding an inline comment to ensure this doesn't get changed in future without awareness of possible escaping issues.
'type' => 'warning', | ||
'additional_classes' => array( 'update-message', 'inline', 'notice-alt' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'type' => 'warning', | |
'additional_classes' => array( 'update-message', 'inline', 'notice-alt' ), | |
'type' => 'warning', | |
'additional_classes' => array( 'update-message', 'inline', 'notice-alt' ), |
$no_customizer_support = __( 'This theme doesn\'t support Customizer.' ); | ||
$no_customizer_support .= '<# if ( data.actions.activate ) { #>'; | ||
$no_customizer_support .= sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$no_customizer_support = __( 'This theme doesn\'t support Customizer.' ); | |
$no_customizer_support .= '<# if ( data.actions.activate ) { #>'; | |
$no_customizer_support .= sprintf( | |
$no_customizer_support = __( 'This theme doesn\'t support Customizer.' ); | |
$no_customizer_support .= '<# if ( data.actions.activate ) { #>'; | |
$no_customizer_support .= sprintf( |
'type' => 'error', | ||
'additional_classes' => array( 'notice-alt', 'notice-large' ), | ||
'attributes' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'type' => 'error', | |
'additional_classes' => array( 'notice-alt', 'notice-large' ), | |
'attributes' => array( | |
'type' => 'error', | |
'additional_classes' => array( 'notice-alt', 'notice-large' ), | |
'attributes' => array( |
$update_incompatible = '<h3 class="notice-title">' . __( 'Update Incompatible' ) . '</h3><p><# if ( ! data.updateResponse.compatibleWP && ! data.updateResponse.compatiblePHP ) { #>'; | ||
$update_incompatible .= sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$update_incompatible = '<h3 class="notice-title">' . __( 'Update Incompatible' ) . '</h3><p><# if ( ! data.updateResponse.compatibleWP && ! data.updateResponse.compatiblePHP ) { #>'; | |
$update_incompatible .= sprintf( | |
$update_incompatible = '<h3 class="notice-title">' . __( 'Update Incompatible' ) . '</h3><p><# if ( ! data.updateResponse.compatibleWP && ! data.updateResponse.compatiblePHP ) { #>'; | |
$update_incompatible .= sprintf( |
); | ||
$update_incompatible .= wp_update_php_annotation( '</p><p><em>', '</em>', false ); | ||
} | ||
$update_incompatible .= '<# } else if ( ! data.updateResponse.compatibleWP ) { #>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$update_incompatible .= '<# } else if ( ! data.updateResponse.compatibleWP ) { #>' | |
$update_incompatible .= '<# } else if ( ! data.updateResponse.compatibleWP ) { #>'; |
And you would've gotten away with it too, if it hadn't been for that pesky semi-colon!
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Tested and works. Currently loses the padding in the error messages due to missing
p
wrapper; todo.Trac ticket: https://core.trac.wordpress.org/ticket/57791
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.