Update Site Health reports copy for more clarity and consistency #272
Conversation
|
@felixarntz This is ready for review with all of the copy changes suggested in #229, as well as some modifications to the autloaded options copy based on your feedback in the previous PR. Apologies for the mess... it's been a very long time since I've worked with Git and I'm getting back into the swing of things! |
| /* translators: 1: Number of autoloaded options. 2.Autoloaded options size. */ | ||
| '<p>' . esc_html__( 'Your website uses %1$s autoloaded options (size: %2$s). Try to reduce the number of autoloaded options or performance will be affected.', 'performance-lab' ) . '</p>', | ||
| /* translators: 1. Number of autoloaded options. 2. Autoloaded options size. */ | ||
| '<p>' . esc_html__( 'Autoloaded options are configuration settings for plugins and themes that are automatically loaded with every page load in WordPress. Having too many autoloaded options can slow down your site. Your site has %1$s autoloaded options (size: %2$s) in the options table, which could cause your site to be slow. You can reduce the number of autoloaded options by cleaning up your site\'s options table.', 'performance-lab' ) . '</p>', |
There was a problem hiding this comment.
See my similar comment on #269, it would be better to break out the first sentence here into its own translation string because it's the exact same sentence between line 49 and here. This makes it easier for translators as it avoids duplicate translations on the same sentence.
Let me know if you prefer any help with that.
There was a problem hiding this comment.
@felixarntz My PHP knowledge is extremely basic at best – the best way that I can find to do this is to define that first sentence ("Autoloaded options are configuration settings...") as a variable like the options count and sizes are, so there would be three within that sentence. That seems like it could still be confusing for translation, though. Is there an easier way?
There was a problem hiding this comment.
@bethanylang I've pushed an additional commit that implements this. Let me know if this looks good to you 👍
There was a problem hiding this comment.
@felixarntz Looks great, thank you for taking care of that!
|
@bethanylang don't forget to upload the copies on the tests also, otherwise, unit testing will keep failing. Besides that, I really like the new wording :) |
|
@manuelRod I've fixed the failing tests, and in the process I also simplified them a bit. Using data providers is helpful when passing different type of data to the same test, but all providers in the test class only had a single data set, which makes it not very practical and harder to follow for no real benefit. I also updated the tests to look for the most important indicating factor to see that the logic in the Site Health check function works, rather than comparing the whole result, which shouldn't really be needed e.g. for copy changes like this the tests should preferably not fail. |
|
looks good @felixarntz ! Thanks for that |
Summary
Fixes #229.
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.