Improve help text for responsive images

Here is a first patch fixing some unneeded HTML inside a string and coding standards

I'm reviewing the patch doing the mentored sprint at Dublin 2016.

Did a review and have the following comments:

1. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -28,7 +28,7 @@ function responsive_image_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('The Responsive Image module provides an image formatter that allows browsers to select which image file to display based on media queries or which image file types the browser supports, using the HTML 5 picture and source elements and/or the sizes, srcset and type attributes. For more information, see the <a href=":responsive_image">online documentation for the Responsive Image module</a>.', array(':responsive_image' => 'https://www.drupal.org/documentation/modules/responsive_image')) . '</p>';
   

   Look at the comment below about possible changes.

2. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -39,14 +39,20 @@ function responsive_image_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<dd>' . t('While you have the option to provide only one image style per breakpoint, the sizes option allows you to provide more options to browsers as to which image file it can display, even when using multiple breakpoints for art direction. Breakpoints are defined in the configuration files of the theme.') . '</dd>';
   

   Look at the comment below about possible changes.

3. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -39,14 +39,20 @@ function responsive_image_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<dd>' . t('Below the Sizes field you can choose multiple image styles so the browser can choose the best image file size to fill the space defined in the Sizes field. Typically you will want to use image styles that resize your image to have options that range from the smallest px width possible for the space the image will appear in to the largest px width possible, with a variety of widths in between. You may want to provide image styles with widths that are 1.5x to 2x the space available in the layout to account for high resolution screens. Image styles can be defined on the <a href=":image_styles">Image styles page</a> that is provided by the <a href=":image_help">Image module</a>.', array(
   

   Look at the comment below about possible changes.

4. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -39,14 +39,20 @@ function responsive_image_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<dd>' . t('After defining responsive image styles, you can use them in the display settings for your Image fields, so that the site displays responsive images using the HTML5 picture tag. Open the Manage display page for the entity type (content type, taxonomy vocabulary, etc.) that the Image field is attached to. Choose the format <em>Responsive image</em>, click the Edit icon, and select one of the responsive image styles that you have created. For general information on how to manage fields and their display see the <a href=":field_ui">Field UI module help page</a>. For background information about entities and fields see the <a href=":field_help">Field module help page</a>.', array(
   

   Look at the comment below about possible changes.

It is looking very good and everything is working. Please considerate to change from concat strings to array and then doing an implode - this is faster. eg:
$output[] = 'something';
$output[] = t('Something translated');
$output[] = 'Something more';
$output = implode('', $output);

You should also replace the array with short tag version '[]'. As by the coding standards for Drupal - this check is already included in the PHPCS. https://www.drupal.org/docs/develop/standards/coding-standards#array

Here is a patch that (I hope) addresses #7 suggestions

I think @pmchristensen also meant the arrays as second parameters to the t() function when he said:

You should also replace the array with short tag version '[]'

Other than that it looks fine to me.

Thanks @Jelle_S fro this clarification

It should say:

"The vw unit is viewport width and is used instead of a percentage because vw always refers to the width of the entire viewport."

instead of:

"The vw unit is viewport width and is used instead of a percentage because the percentage always refers to the width of the entire viewport."

Re-rolled patch to be up to date with 8.4.x.

Updated the help text to replace 'percentage' with 'vw' as noted in #12.

Re-rolled patch to be up to date with 8.5.x.

I think the tags used in this issue are somewhat overkill, since it just a documentation change. However the new text looks solid and was approved by @Jelle_S in #9.

Temporary testbot hiccup.

+++ b/core/modules/responsive_image/responsive_image.module
@@ -42,29 +42,29 @@
+      $output[] = '<dd>' . t('Once the sizes option is selected, you can let the browser know the size of this image in relation to the site layout, using the <em>Sizes</em> field. For a hero image that always fills the entire screen, you could simply enter 100vw, which means 100% of the viewport width. For an image that fills 90% of the screen for small viewports, but only fills 40% of the screen when the viewport is larger than 40em (typically 640px), you could enter "(min-width: 40em) 40vw, 90vw" in the Sizes field. The last item in the comma-separated list is the smallest viewport size: other items in the comma-separated list should have a media condition paired with an image width. <em>Media conditions</em> are similar to a media query, often a min-width paired with a viewport width using em or px units: e.g. (min-width: 640px) or (min-width: 40em). This is paired with the <em>image width</em> at that viewport size using px, em or vw units. The vw unit is viewport width and is used instead of a percentage because vw always refers to the width of the entire viewport.</dd>');

The </dd> is not supposed to be in the translated string.

Also the conversion here to use $output as an array feels very unnecessary - speed is really not of the essence here and if it is measurable it's not going to be significant - it also hides actual changes to the text - of which there does not appear to be very many.

Updated as of #21

I am at the Drupalcon Nashville 2018 mentored sprint, and am going to try to review the latest patch.

Ooook.. here we go:

1. Enable responsive image module.
2. Locate where this text currently lives... found it, help page.
3. Page looks obtuse, currently.. but my ability to review this may be limited by my lack of experience working with responsive images. Focusing on the other aspects: readability, formatting, no obvious visual problems.
4. Applying patch.
5. Went back to review code, and found this:

Line 54 still has a </dd> in the translated string. Fixing and uploading new patch momentarily. I'm very new to providing patches, so I might screw it up.. but gotta start somewhere.

Everything else looks good, although I cannot realistically vet the quality of the content.

After wrapping my head around the fact that I'm actually reverting line 54 to it's HEAD state on 8.6.x (so. confusing.).. I finally managed to produce this patch. Crossing my fingers!

There is one extra space after the opening bracket in [ ':responsive_image.

So confusing.. looks like that's yet another reversion to HEAD. I hope I'm doing this right.. new patch uploaded!

The patch no longer applies to the latest. There also seems to be some differences between the latest several patches here, but I couldn't figure out why some changed multiple lines of the help text, and the latest only changes one line of actual help text.

+++ b/core/modules/responsive_image/responsive_image.module
@@ -42,8 +42,7 @@
-      $output = '';
-      $output .= '<h3>' . t('About') . '</h3>';
+      $output = '<h3>' . t('About') . '</h3>';

This change seems out of scope for this. Most core modules follow the pattern of initiating $output to an empty string.

The patch in #32 looks good, as it resolves the bug in the current help text first noted in #12. However, the IS states a more thorough rewrite should happen:

The help text in general (both field descriptions and actual help page) does not conform to UI guidelines and needs clean-up. Specifically, it's all way too long, and it focuses on exposing the underlying technical details vs. being more use-case focused "Use this to foo your bar so your users can baz." See Field UI settings descriptions for an example.

which seems to have been lost.

Since this does address an actual bug in the help text, it seems good to get in, but a follow-up should be filed, and the IS here should be updated I think.

Closed #2887404: responsive_image_help words mixed up as a duplicate. Adding credit for kari.kaariainen.

Setting to NW for the points in #33. Once the followup is made and the IS update this can be RTBC'ed.