-
Notifications
You must be signed in to change notification settings - Fork 799
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
Debug Tests: Improve connection test, and allow overrides #14740
Conversation
- Allow successful tests to provide their own message and label for the Site Health page. - Adjust the message and label for the `test__check_if_connected` test
- Adds the ability for tests to supply an action label and override other labels. - Improves the failing state for `test__check_if_connected`
This is an automated check which relies on |
Allow passing tests to override defaults with an HTML description.
- perserve plain text messages to use on API tests - allow tests to override plain text with HTML for the Site Health page.
Thanks for the review @kraftbj - i had no idea that these tests were also used in CLI. I've made changes to preserve CLI functionality while allowing overrides for HTML in Site Health |
sprintf( | ||
'<p>%s</p><p>%s</p>', | ||
__( 'A healthy connection ensures Jetpack essential services are provided to your WordPress site, such as Stats and Site Security.', 'jetpack' ), | ||
__( '✅ Your site is connected to Jetpack.', 'jetpack' ) |
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.
Maybe we should pull the emoji out of the translation and use a placeholder instead? I am not sure translators / translator tools can all handle adding emoji.
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.
Agreed — ideally this should reuse a Dashicon instead of being an emoji:
Green/Healthy/Good:
<span class="dashicons pass"><span class="screen-reader-text">Passed</span></span>
Read/Error/Bad:
<span class="dashicons fail"><span class="screen-reader-text">Error</span></span>
__( 'Your site is not connected to Jetpack', 'jetpack' ), | ||
__( 'Learn more about this process', 'jetpack' ), | ||
sprintf( | ||
'<p>%s</p><p>%s<strong>%s</strong></p>', |
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.
Just a nit-pick, but I personally find it easier to read when we use numbered placeholders when we have more than a couple of them. Up to you!
* | ||
* @return array Test results. | ||
*/ | ||
public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical' ) { | ||
public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = 'Resolve', $description = false ) { |
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.
Maybe we can pull Resolve
out of there so it can still be translated?
public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = 'Resolve', $description = false ) { | |
public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = '', $description = false ) { | |
if ( empty( $action_label ) ) { | |
$action_label = __( 'Resolve', 'jetpack' ); | |
} |
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.
Looks good to me! I've changed the failed state a tiny bit in iteration no. 2 though, which will be posted shortly.
Designs i2 posted p6TEKc-3t2-p2 |
- adds dashicons instead of emoji - improves translation logic
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.
It tests well for me. 👍
A couple of comments:
Having multiple strings show up in different places could complicate seeking support or answers if it isn't clear that multiple messages are referring to the same thing. |
…e-added as part of next sprint when multiple action links are supported
Per the i2 designs the primary action text should be "Reconnect your site now" for failed connections. |
Note about changelog we could use something around the lines of: Site Health Tests |
* 8.3 release: changelog * Changelog: add #14516 * Changelog: add #14574 * Bring in changes from 8.2.1 and 8.2.2 * Update stable version * Bring in 8.2.3 changes * Changelog: add #14714 * Changelog: add #14639 * Changelog: add #14678 * Changelog: add #14673 * Changelog: add #14687 * Changelog: add #14704 * Changelog: add #14702 * Changelog: add #14541 * Changelog: add #14657 * Changelog: add #14622 * Changelog: add #14582 * Changelog: add #14638 * Changelog: add #14633 * Changelog: add #14571 * Changelog: add #14592 * Changelog: add #14539 * Changelog: add #14514 * Changelog: add #14643 * Changelog: add #14494 * Changelog: add #13739 * Changelog: add #14707 * Changelog: add #14736 * Changelog: add #14706 * Changelog: add #14730 * Changelog: add #14685 * Changelog: add #14727 * Changelog: add #14711 * Changelog: add #14742 * Changelog: add #14746 * Changelog: add #14725 * Changelog: add #13999 * Changelog: add #14740 * Changelog: add #14759 * Changelog: add #14703 * Changelog: add #14753 * Changelog: add #14754 * Changelog: add #14645 * Cahngelog: add #14599
Changes proposed in this Pull Request:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
Proposed changelog entry for your changes: