Review hook_help for Picture module

Agreed. Added a link to the module documentation at http://drupal.org/documentation/modules/picture and updated a call to the url() function.

#2: picture-hook_help_urls-2033415-2.patch queued for re-testing.

one small thing, the tokens for routes should use ! rather than @

'@link' becomes '!link'

We are not splitting the t() call into multiple lines in any other hook_help() implementations, so let's not do it here either?

This is fine.... But did someone review the rest of the existing help? I took a quick look... It seems like the "Formatting an Image field" Uses bullet ought to have a link to the Image module help?

Also, just reading the complete help over... I'm assuming that this module would only be useful if you can define multiple sets of Picture mappings? What I mean is, you might have a "large" mapping, which would use Image Styles of 400x400, 300x300, 200x200, and 100x100 at different breakpoints, and then you might have a "gallery" mapping, which would use image styles of 300x200, 150x100, and 75x50 at different breakpoints. Then on the Image display settings, you would choose to use the "large" or "gallery" picture style.

At least, I'm imagining that is how it works? I can't imagine it would be useful if you could only define one set of breakpoints for images. If that is the case, then I think the help text for this module is misleading, because it seems to imply that there can only be one set of mappings, or at least it doesn't say that you can have multiple mappings.

What are these mapping sets called, anyway in the UI?

I imagine this would be better if the Uses section had items:

- Adding a mapping set [or whatever these are called in the UI]
- Defining image styles for a mapping set [this would be the existing "Mapping image styles to breakpoints" item]
- Using a mapping set to format an Image field [this would be the existing "Formatting an image field" item, but would refer to the need to choose a mapping set or whatever they are called]

This seems very clear to me!

A few minor things to fix:

a) When you link to the Breakpoint module help... what we've done in other help texts is just make the first mention of the other module into a link to that module's help page. So let's do that here and get rid of the "See the help text for the Breakpoint module" sentence.

b) "content type, user, taxonomy term et cetera" --> Should be "content type, taxonomy vocabulary, etc." -- I think we should get rid of "user" because it's not like the other entities and you'd need to say something cumbersome like "all user accounts" or ... it's just weird and makes things confusing.

c) "Field UI help" ==> "Field UI module" or "Field UI module help".

d) In the last Uses item... We've been very careful in the individual field help texts to standardize how we talk about field settings and display settings. So maybe here we should say something like "After defining picture mappings, you can use them in the display settings for your Image fields, ...".

e) Don't use the word "Drupal" in UI text. See
https://drupal.org/node/604342#wording

OK. I think your changes are mostly great!

One place I'm still not sure of:

<a href="!entity_help">entity type</a>

For accessibility reasons, links need to have either text telling where the link goes, or a title telling where it goes. The link text here is "entity type", which doesn't match "this goes to the Entity module help page". So I think we need to add a link title here.

Or, maybe the best thing would be to take the link out there, and combine it with the "For more information" sentence that points to Field UI at the end of that paragraph -- like adding "... and for general information about entities blah blah blah" to that last sentence?

Excellent! I think this is ready to go. But it needs a quick manual test:
- Make sure all the links work
- Make sure where things are mentioned in the Drupal user interface, that they match what you see there
- Make sure the formatting is OK.

I gave this a manual review today, and it all looks great except:

a) Uses / Define:
- I don't think we need two links there to the same admin page, especially with different link text? Let's get rid of the first one.
- Technically the link says "Add picture mapping" not "Add a picture mapping", and I think you would click the link rather than "choosing" it?
- I tried this out in the UI and after creating a label and choosing a breakpoint group, I have to click Save in order to map to image styles. This is not clear in the help.

b) Uses / Using:
- The field type is called "Image" not "image".
- After choosing the format type Picture, I have to click the configure button in order to select the picture mapping. This is not clear from the help. Maybe it should just use our standard language about "in the display settings", since it also has a link to the Field UI module?

20: picture-hook_help_urls-2033415-20.patch queued for re-testing.

Agreed. I think this is looking good! Needs a manual test:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

Patch is working fine. admin/config/media/picturemapping link is not working.

Everything seems OK to me, @sandipmkhairnar perhaps you did not have the Picture module enabled?
I see no reason why this needs any additional work.

RE #26 - I don't think you can see the Picture module help unless the Picture module is enabled, so if this link is not working, there is most likely a problem in the hook_help()?

But that said, I just gave the patch a manual test myself. All of the links work, and I am also seeing the proper help text at the top of admin/config/media/picturemapping (which wasn't touched by this patch). All the formatting is good and the text in the help correctly describes what I see in the UI. I think this one is ready to go!

Thanks again! Committed to 8.x.