Background:
This issue is part of the task to update/create the hook_help texts of the modules for Drupal 8:
#1908570: [meta] Update or create hook_help() texts for D8 core modules

Tasks:
- write the hook_help function
- review d.o. docs at http://drupal.org/documentation/modules/picture

Files: 
CommentFileSizeAuthor
#20 picture-hook_help_urls-2033415-20.patch3.66 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 63,756 pass(es).
[ View ]
#20 interdiff.txt4.03 KBbatigolix
#16 interdiff.txt2.9 KBbatigolix
#16 picture-hook_help_urls-2033415-16.patch3.62 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 59,795 pass(es).
[ View ]
#14 interdiff.txt3.82 KBbatigolix
#14 picture-hook_help_urls-2033415-14.patch3.55 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 59,886 pass(es).
[ View ]
#12 interdiff.txt3.61 KBbatigolix
#12 picture-hook_help_urls-2033415-12.patch3.51 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 59,809 pass(es).
[ View ]
#10 picture-hook_help_urls-2033415-10.patch1.99 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 59,133 pass(es).
[ View ]
#7 picture-hook_help_urls-2033415-7.patch1.71 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 58,634 pass(es).
[ View ]
#2 picture-hook_help_urls-2033415-2.patch1.71 KBtankerjoe
FAILED: [[SimpleTest]]: [MySQL] 58,677 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Comments

batigolix’s picture

It needs at least a link to the on line docs

tankerjoe’s picture

Status:Active» Needs review
StatusFileSize
new1.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,677 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, picture-hook_help_urls-2033415-2.patch, failed testing.

tankerjoe’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, picture-hook_help_urls-2033415-2.patch, failed testing.

lostkangaroo’s picture

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

'@link' becomes '!link'
batigolix’s picture

StatusFileSize
new1.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,634 pass(es).
[ View ]

Patch changes '@link' to '!link'

batigolix’s picture

Status:Needs work» Needs review

Change status

jhodgdon’s picture

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

batigolix’s picture

StatusFileSize
new1.99 KB
PASSED: [[SimpleTest]]: [MySQL] 59,133 pass(es).
[ View ]

fixed

jhodgdon’s picture

Status:Needs review» Needs work

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]

batigolix’s picture

Component:documentation» picture.module
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,809 pass(es).
[ View ]
new3.61 KB

I now had a first real look at the actual text. The patches before were only dealing with token and url.

As for your questions in #11. I cannot answer them but I checked how the Picture module works and I think the new patch covers it well (hopefully). I used Heathers blog for inspiration.

I spent quite some time moving small bits of text around, which often leads to errors. So a good review is required.

I also assign to picture.module component for feedback.

jhodgdon’s picture

Status:Needs review» Needs work

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

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new3.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,886 pass(es).
[ View ]
new3.82 KB

Okay, new attempt.

Regarding #13:

a) Yes, but. There is no explicit mentioning of the Breakpoint module. I could put a link on the 1st occurrence of the word breakpoint, but not sure if it is explicit enough for the reader to know what link he is going to follow. I had a similar situation with Entity Ref
b) OK
c) OK-ish. I went with help page for the XYZ module
d) OK
e) OK

jhodgdon’s picture

Status:Needs review» Needs work

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?

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new3.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,795 pass(es).
[ View ]
new2.9 KB

Agree. Let's try this then

jhodgdon’s picture

Issue tags:+Needs manual testing

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.

batigolix’s picture

Issue tags:+Novice
jhodgdon’s picture

Status:Needs review» Needs work

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?

batigolix’s picture

Status:Needs work» Needs review
Parent issue:» #1908570: [meta] Update or create hook_help() texts for D8 core modules
StatusFileSize
new4.03 KB
new3.66 KB
PASSED: [[SimpleTest]]: [MySQL] 63,756 pass(es).
[ View ]

Thanks for the review.

This patch addresses the points from #19.

About:

Maybe it should just use our standard language about "in the display settings", since it also has a link to the Field UI module?

Picture is not really a field module. But it adds a configuration option to the display settings. The standard text (the one we use in Link module?) is a bit too general for this?

Status:Needs review» Needs work

The last submitted patch, 20: picture-hook_help_urls-2033415-20.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Needs review
jhodgdon’s picture

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.

sandipmkhairnar’s picture

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

batigolix’s picture

Status:Needs review» Needs work
m1n0’s picture

Status:Needs work» Needs review

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.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing, -Novice

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!

jhodgdon’s picture

Status:Reviewed & tested by the community» Fixed

Thanks again! Committed to 8.x.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Eli-T’s picture

Component:picture.module» responsive_image.module