Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#46734 closed enhancement (fixed)

Site Health: Add actions to all site status tests

Reported by: garrett-eclipse's profile garrett-eclipse Owned by: desrosj's profile desrosj
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: site-health has-patch commit
Focuses: ui, docs Cc:

Description

Hello,

It would be nice to include links to the themes/plugins/update screens where applicable such as the 'Inactive plugins should be removed' and 'You should remove inactive themes' improvements.

Thank you

Attachments (14)

Screen Shot 2019-03-30 at 12.54.08 AM.png (178.9 KB) - added by garrett-eclipse 5 years ago.
Current improvements for inactive plugins/themes
Screen Shot 2019-03-30 at 1.08.32 AM.png (213.2 KB) - added by garrett-eclipse 5 years ago.
Suggested button inclusion
46734.patch (7.7 KB) - added by Clorith 5 years ago.
46734.diff (8.1 KB) - added by vaishalipanchal 5 years ago.
Updated patch as per above suggestions.
46734.2.diff (7.8 KB) - added by garrett-eclipse 5 years ago.
Minor grammar fix, and caught another 'host'
46734.3.diff (8.9 KB) - added by garrett-eclipse 5 years ago.
Updated patch to address feedback
Screen Shot 2019-04-07 at 11.46.29 PM.png (111.5 KB) - added by garrett-eclipse 5 years ago.
Inactive Plugins w/ Actions
Screen Shot 2019-04-07 at 11.46.43 PM.png (97.0 KB) - added by garrett-eclipse 5 years ago.
Inactive themes w/ action
Screen Shot 2019-04-07 at 11.52.38 PM.png (63.3 KB) - added by garrett-eclipse 5 years ago.
Your site not HTTPS w/ updated link to be external
46734.4.diff (9.6 KB) - added by garrett-eclipse 5 years ago.
Translated all help links, included translator comments for links, updated external icon via css to remove underline
Screen Shot 2019-04-08 at 1.03.18 PM.png (57.2 KB) - added by garrett-eclipse 5 years ago.
Screen illustrating the external Icon w/o underline
46734.5.diff (9.6 KB) - added by garrett-eclipse 5 years ago.
Updated patch to restore the 'Manage inactive plugins' string and be less specific with the css, also with the CSS selected went with the generic .dashicons over .dashicons-external to future proof.
Screen Shot 2019-04-11 at 10.11.37 AM.png (43.9 KB) - added by garrett-eclipse 5 years ago.
Screen to illustrate restored string
46734.6.diff (9.6 KB) - added by garrett-eclipse 5 years ago.
Removed extra newline to fix CS issue flagged by @Clorith

Download all attachments as: .zip

Change History (36)

@garrett-eclipse
5 years ago

Current improvements for inactive plugins/themes

@garrett-eclipse
5 years ago

Suggested button inclusion

#1 @garrett-eclipse
5 years ago

Not sure if it would need to open in a new tab, the external icon mainly got in there as I copied the button from the PHP update recommendation.

#2 @garrett-eclipse
5 years ago

  • Version set to trunk

#3 @Clorith
5 years ago

  • Keywords site-health added
  • Milestone changed from Awaiting Review to 5.2
  • Summary changed from Site Health: Add buttons for 'Manage plugins' and 'Manage themes' to the inactive improvements to Site Health: Add actions to all site status tests

This is something we were intending to do, all tests should have actions linking off to either documentation, or pages to resolve the reported problems.

Quick remark that we should ensure they are links as long as they direct away from the current page you are on, as recommended by the accessibility team.

Related #46573

@Clorith
5 years ago

#4 follow-up: @Clorith
5 years ago

46734.patch starts to introduce actions for various tests.

There are a few missing, and I'm not quite sure what resources they could link to that would make sense. Some have helpful texts, directing the user to contacting their web host (for example to update PHP, update SQL, or missing PHP libraries), while others are a bit more vague as to what resources should be used, for example:

  • Your site can perform loopback requests
  • Your site can communicate securely with other services
  • Scheduled events are running (or failing)

These are just a few of the ones missing actions, there are more, especially if you consider potential failure states as well.

I know what these mean, the short description following each will hopefully help users understand as well, but these are all things that the average user may find hard to troubleshoot, and in many cases end up being "Contact your host", which we of course have no direct way of linking to.

So I guess the overarching question is, should we just omit actions in these cases, or should we (like I did for one of them) link to the WordPress support forums with a "get help" kind of text?

#5 in reply to: ↑ 4 @garrett-eclipse
5 years ago

  • Keywords has-patch needs-refresh added

Replying to Clorith:

46734.patch starts to introduce actions for various tests.

Thanks for initial patch there @Clorith

There are a few missing, and I'm not quite sure what resources they could link to that would make sense. Some have helpful texts, directing the user to contacting their web host (for example to update PHP, update SQL, or missing PHP libraries), while others are a bit more vague as to what resources should be used, for example:

  • Your site can perform loopback requests
  • Your site can communicate securely with other services
  • Scheduled events are running (or failing)

These are just a few of the ones missing actions, there are more, especially if you consider potential failure states as well.

I know what these mean, the short description following each will hopefully help users understand as well, but these are all things that the average user may find hard to troubleshoot, and in many cases end up being "Contact your host", which we of course have no direct way of linking to.

So I guess the overarching question is, should we just omit actions in these cases, or should we (like I did for one of them) link to the WordPress support forums with a "get help" kind of text?

For these more superfluous tests if there's nothing to link to then I feel it's fine to leave especially if it's a 'contact your host' as the call to action is in the verbiage and don't have a link we can determine.

However, if there is further information that could be provided potentially as a HelpHub page that could be very useful to the user and the interface as it could help reduce the content in the accordion while providing more information and potential troubleshooting steps via an external article.

Reviewing the patch some minor thoughts;

  1. 'please contact your host' can this be 'please contact your host or developer' as in many cases the person who would deal with the change is their developer or agency and not the host themselves.
  2. Should the URL 'https://wordpress.org/support' be translatable?
  3. Can the string 'Remove inactive plugins' be updated to 'Manage inactive plugins' to avoid hesitation. Reading 'Remove inactive plugins' I didn't want to click it thinking it may conduct that action for me when I would prefer to review the list and then remove which is what is happening.

Thanks for the amazing work @Clorith stoked to see Site Health making it's way to core.

#6 @mukesh27
5 years ago

  • Focuses docs added

@Clorith thanks for your patch and @garrett-eclipse thanks for review that patch.

Reviewing the patch and found some minor changes that need to fix also.

  1. When use sprintf function with multiple %s then it is better to use %1$s and %2$s respectively
  2. Use a consistent wording 'please contact your host' should be 'Please contact your server administrator.'

@garrett-eclipse as per core software URL 'https://wordpress.org/support' is not translatable.

Manage inactive plugins text is good option.

Last edited 5 years ago by mukesh27 (previous) (diff)

@vaishalipanchal
5 years ago

Updated patch as per above suggestions.

#7 @vaishalipanchal
5 years ago

  • Keywords needs-refresh removed

@garrett-eclipse
5 years ago

Minor grammar fix, and caught another 'host'

#8 @garrett-eclipse
5 years ago

  • Keywords needs-testing added

Thanks for the feedback @mukesh27 and patch @vaishalipanchal. I uploaded 46734.2.diff to replace the comma preceding the new 'Please...' with a period as well as updated one more instance of your host with your server administrator, I do like that term it's more encompassing.
Cheers

#9 follow-up: @Clorith
5 years ago

The reason you want to use %1$s and similar is if there are multiple attributes within a translation string that are better suites in a different order. Here the order is fixed, the first element (the link) always has to go inside the a tag and the second one, the string, is always the second one. Adding ordered replacements here add a potential failure element if used incorrectly so as the coding standards don't state that they must be used I'd prefer the use of the fixed format when possible, just to be on the safe side.

@garrett-eclipse I agree with you on the wording, it sounds less frightening that way to a regular user without technical experience.

We should also make the URL to the support area translatable as suggested.

@garrett-eclipse
5 years ago

Updated patch to address feedback

@garrett-eclipse
5 years ago

Inactive Plugins w/ Actions

@garrett-eclipse
5 years ago

Inactive themes w/ action

@garrett-eclipse
5 years ago

Your site not HTTPS w/ updated link to be external

#10 in reply to: ↑ 9 @garrett-eclipse
5 years ago

Thanks @Clorith I appreciate the review/feedback.

Replying to Clorith:

The reason you want to use %1$s and similar is if there are multiple attributes within a translation string that are better suites in a different order. Here the order is fixed, the first element (the link) always has to go inside the a tag and the second one, the string, is always the second one. Adding ordered replacements here add a potential failure element if used incorrectly so as the coding standards don't state that they must be used I'd prefer the use of the fixed format when possible, just to be on the safe side.

This makes sense for sure, might be nice to get that into the coding standards, within my updated patch I've set all static strings to use %s and left the translatable strings using the %1$s convention.

We should also make the URL to the support area translatable as suggested.

I've updated the support URL to make it translatable and searched core to find all references I could and they were all translatable.

And one final addition with the actions being fairly consistent I updated the 'Read more about why you should use HTTPS' link to use the convention making it an external link w/ the icon.

Would love another review and testing, uploaded screens to illustrate changes.

Other notes;

  • There's no filter for inactive themes hence why it doesn't have the same second action link that plugins has.
  • The 'One or more recommended modules are missing' section has a link to the 'the team handbook', should this be an action link?
  • Is there a handbook URL for 'Background updates are not working as expected'?
  • Which WP.org links should be translatable? Some like support make sense but others don't have variants so could be removed from the load on translators. Below is recon on this for class-wp-site-health.php;

Translatable;
https://make.wordpress.org/hosting/handbook/handbook/server-environment/#php-extensions
https://wordpress.org/about/requirements/
https://wordpress.org/support
https://wordpress.org/support/article/why-should-i-use-https/
Not Translatable;
https://wordpress.org/support/article/debugging-in-wordpress/

#11 @Clorith
5 years ago

They should all be translatable, HelpHub has stuff in the works and once localized versions start coming out we can then easily make them available without needing core updates.

We don't have a handbook page for the background updates not working, there's a "something in your setup is wrong" scenario, there's multiple potential failures, so telling them to reach out to their server admin is the best approach there.

For the team handbook link, that's intended to be a part of the description (it's just letting you know where the reason and data is sourced from), so it shouldn't be an action link (since it's not an actionable area, it's merely contextual).

I approve of the inclusion of an external indicator, we should probably check with design on those since the icons are huge and really break the link string in my opinion (just removing the underline would do the trick I suspect, but I don't want to mess with existing conventions without them seeing that it's OK to do so).

This ticket was mentioned in Slack in #design by clorith. View the logs.


5 years ago

#13 @Clorith
5 years ago

After a quick discussion in the design channel, let's remove the link-underlines from the icons here.

@garrett-eclipse
5 years ago

Translated all help links, included translator comments for links, updated external icon via css to remove underline

@garrett-eclipse
5 years ago

Screen illustrating the external Icon w/o underline

#14 @garrett-eclipse
5 years ago

Thanks @Clorith I appreciate the feedback and for raising that with design.

In 46734.4.diff, I've translated the last URL as well as provided translator comments for all of the existing links. And added css to remove the underline on the external icon.

Please review,
Cheers

#15 @Clorith
5 years ago

  • Keywords has-patch needs-testing removed

46734.4.diff looks good code-wise, but the string changes to "Manage inactive plugins" and such are undone, once those are fixed this should be good to go.

#16 @mukesh27
5 years ago

  • Keywords needs-refresh added

As per WPCS https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors Refrain from using over-qualified selectors, div.container can simply be stated as .container

@garrett-eclipse Can you please replace below css or add unique css in span.

.health-check-accordion-panel a span.dashicons-external {
	text-decoration: none;
}

Replace to

.health-check-accordion-panel a .dashicons-external {
	text-decoration: none;
}

@garrett-eclipse
5 years ago

Updated patch to restore the 'Manage inactive plugins' string and be less specific with the css, also with the CSS selected went with the generic .dashicons over .dashicons-external to future proof.

@garrett-eclipse
5 years ago

Screen to illustrate restored string

#17 @garrett-eclipse
5 years ago

  • Keywords has-patch needs-testing added; needs-refresh removed

Thanks for catching that string reversion @Clorith I've caught that but did a scan and feel that's the only one amiss, can you recheck please.

And thanks @mukesh27 for catching that CS issue, I'm learning those rules more each day ;)
The updated patch makes the selector less specific as well I updated to use just .dashicons and not .dashicons-external to future proof in case other dashicons are used in any of the links.

Can you guys please retest/review and we'll get this in before the string freeze (hopefully).

Cheers

#18 @Clorith
5 years ago

  • Keywords commit added

Let's roll with this, there's a WPCS error, but it can be automatically fixed, and doesn't relate to strings, so let's get this in.

@garrett-eclipse
5 years ago

Removed extra newline to fix CS issue flagged by @Clorith

#19 @garrett-eclipse
5 years ago

  • Keywords needs-testing removed

Thanks @Clorith for flagging that, I used the issue to learn how to setup wpcs/phpcbf so was able to identify and remove the extra newline there. Uploaded 46734.6.diff to address the WPCS error.

#20 @ianbelanger
5 years ago

46734.6.diff looks good @garrett-eclipse, unless there is anything else, I say we ship this

#21 @desrosj
5 years ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 45170:

Site Health: Add missing actions to tests.

This change adds missing actions for several tests. This ensures that the user is provided with a next step, whenever possible.

Also, change the URL displayed in the WordPress.org communication test description to api.wordpress.org for accuracy.

Props: garrett-eclipse, Clorith, vaishalipanchal.
Fixes #46734.

#22 @spacedmonkey
5 years ago

  • Component changed from Administration to Site Health
Note: See TracTickets for help on using tickets.