Background:
This issue is part of the task to update the hook_help texts of the Drupal 8 modules:
#1908570: [meta] Update or create hook_help() texts for D8 core modules
Tasks:
a) Review / write the hook_help text according to help guidelines
b) (novice) Final manual testing:
1. Apply the patch.
2. Go to admin/help.
3. Click on the help page for this module (Comment).
4. Verify that the help page is OK:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.
Comment | File | Size | Author |
---|---|---|---|
#30 | comment-updated_help_d8-2091299-30.patch | 4.56 KB | amitgoyal |
Comments
Comment #1
berkas1 CreditAttribution: berkas1 commentedTaking care of this - during DC Prague sprint.
Comment #2
berkas1 CreditAttribution: berkas1 commentedComment #4
berkas1 CreditAttribution: berkas1 commentedComment #5
berkas1 CreditAttribution: berkas1 commentedComment #6
joverThe patch from #4 works.
Comment #7
alexpottPatch no longer applies.
Comment #8
dbazuin CreditAttribution: dbazuin commentedDone a rerole
Comment #9
dbazuin CreditAttribution: dbazuin commentedtagging
Comment #10
dbazuin CreditAttribution: dbazuin commentedtagging
Comment #12
berkas1 CreditAttribution: berkas1 commentedpath patched :D
Comment #13
jhodgdonThanks for the patches!
There are a few additional problems with this help text -- it is still not quite following our Drupal help standards http://drupal.org/node/632280 or general text standards https://drupal.org/style-guide/content or generic spelling/grammar/punctuation:
a) The spelling of "customized" is sometimes "customised". We should use the Z since we use American Engilsh as our Drupal standard.
b) The punctuation is incorrect in the first Uses item:
- "e.g." should always be punctuated like this:
blah blah blah; e.g., blah blah blah
- There is a clause that says "a content type", which needs to end with a comma.
c) Some of the links are not up to accessibility standards. Link text needs to explain where the link goes, and if it doesn't, there needs to be a link title. For instance:
a content <a href='!content-type'>type</a>
is not an accessible link, because "type" doesn't tell where the link goes.d) We discussed on another issue how to refer to entities, and this module is not using the standard language that we agreed to. See
#2030569-23: [policy] Decide how to refer to "entities" and "bundles" in D8 UI
Specifically, comment #23 has suggestions for field modules and the Field UI module. This seems like it should be similar to the Field UI module, so we should say something similar to this but appropriate to the Comment module:
The Field UI module allows you to create and attach fields to fieldable sub-types of entity types (entity types include content items from the Node module, with content types being the sub-types; taxonomy terms from the Taxonomy module, with vocabularies being the sub-types; and user accounts, with no sub-types).
I am actually not sure which entity types the Comment module now allows you to attach to... do they need to be fieldable? Can you attach comments to comments (hopefully not)?
e) According to our help standards, items in Uses should have headings with "-ing" verbs. The second one here doesn't.
Comment #14
InternetDevels CreditAttribution: InternetDevels commentedFixed issues from previous comment.
Comment #15
jhodgdonLooking better -- thanks for activating this issue again!
I noticed a few things that I think still need attention:
a) One place in About the module is called "Comments" instead of "Comment".
b) In About:
"The Comments module provides possibility to create and attach comment field to fieldable sub-types of entity types..."
That's really awkward wording. And is commenting really a field? I see that it's managed like a field, but I doubt users would see it as really being a "field".
So how about if we said something like "On the Manage fields page for an entity sub-type, you can enable commenting by adding a Comment field..." ?
c) At end of About, this still doesn't have our standard reference to the on-line docs. See http://drupal.org/node/632280
d) There is still some information in Uses that refers specifically to content types -- it shouldn't, since comments can now be enabled for other entities. And some of the information in Uses is redundant to what is in the About section.
So I think that once we explain in About what an entity sub-type is, we can (and should) then use that term consistently in the rest of the help, without explaining it again? I also think that we should avoid saying "attach" for turning on commenting ability for an entity sub-type, because that is really only sensible terminology if you are a Field API programmer and doesn't really make sense in the UI. Just say things like "Commenting can be enabled" and "If comments are enabled", etc.
e) There's some text in Uses that says anyone can edit their own comments until there's been a reply. But this is not really true -- there is an "Edit own comments" permission that a person would have to have in order to do that.
Comment #16
jhodgdonComment #17
jhodgdonWe just had a change to hook_help, on this issue: #2183113: Update hook_help signature to use route_name instead of path.
Here is the change record: https://drupal.org/node/2250345
This patch will need a reroll for this change.
Comment #18
amitgoyal CreditAttribution: amitgoyal commented@jhodgdon - I have addressed all the issues mentioned by you in #15. #17 is already taken care. The last patch #14 was also not working with current code.
Please review attached patch for all these fixes.
Comment #19
jhodgdonThanks for the new patch!
Reference on hook_help standards: http://drupal.org/node/632280
I took a look at this patch. It looks pretty good, but it still needs a little bit of work:
a) The wording of the reference to the on-line help needs an update to standards (in About at end).
b) We also have more standard wording about entities on several of the field modules, and I think this patch is a little careless in places about the difference between entity types, entity sub-types (bundles), and entity items. You might look at Entity Reference, Number, Options, Telephone, and Text, as all of these modules' help has been updated.
Comment #20
amitgoyal CreditAttribution: amitgoyal commented@jhodgdon - Thanks for your feedback!
a) I have updated the on-line help wording as per the hook_help standards.
b) I have also made some changes based on the help text in Entity Reference module. Please see if it looks good otherwise it would be great if you can suggest the text.
Comment #21
jhodgdonThe new wording looks much better, thanks!
A couple of things to fix:
a)
This is still not quite right. The link text (a tag) should start at "online documentation..." not at "Comment".
b) All HTML attributes should be using "" not ' such as:
You may need to change the t() to use single quotes, and escape apostrophes like user\'s or whatever.
c)
Punctuation: ... The "e.g., a content type" should probably be in parentheses here. And it would be preferable to use "for example" instead of "e.g.".
d) There is still confusion between entity items, entity types, and entity sub-types (which are programmatically called "bundles" but we do not want to use that term in the help!). So:
1.
This is not right. You need to explain entity types and sub-types somewhere in the help (as is done in other field modules I think?), and in this sentence, the entity sub-types each have their own default settings.
2.
I don't think I'd call it "content", it's "entity items"
3.
No, again they are customized for each entity sub-type. Content type is specific for Node entities only, so do not use this terminology.
4.
item of content ---> entity item
Also... this whole section is pretty repetitive. We should just say once that the default settings can be made for each entity sub-type and overridden for each entity item, and then talk about what the settings are, not make the same statement about default/override for each setting separately.
e)
I do not think this belongs in the Configuring default and custom settings uses paragraph at all? Should be somewhere else.
f)
This is not a good title for this Uses item. Maybe "Approving and managing comments"?
Comment #22
amitgoyal CreditAttribution: amitgoyal commentedThanks for detailed feedback @jhodgdon!
a) The link text (a tag) is now starting at "online documentation..." .
b) All HTML attributes are now using "".
c) Punctuation corrected and for example wording has been used.
d) Entity word has been linked it's module help page. Repetitive wording has been removed. Text corrections have been done.
e) Text line has been shifted to "Approving and managing comments" as various permissions are refereed there.
f) New title "Approving and managing comments" looks good and has been changed.
Comment #23
jhodgdonBetter, thanks!
Still some things to be fixed:
a) All links to drupal.org should be https
b)
This got missed -- again, you enable comments not for "entity" but for entity sub-types, and please do not use the word Drupal here. Also it doesn't say any more that the way to enable comment functionality is to add a comment field. It should.
c) Still repetitive:
d) Maybe this whole thing would be clearer if we had one Uses point called "Enabling commenting and configuring defaults" (which would describe how to add the field and the configuration options) and a second one for "Overriding default settings" (which would tell us that on each entity item, you can override the defaults, and that after an item is created, changing the defaults will not affect the individual items)?
Comment #24
amitgoyal CreditAttribution: amitgoyal commented@jhodgdon - I have tried to make changes as per your feedback in a, b, c & d. Please see if they look good to you.
Comment #25
jhodgdonLooks pretty good! A few very minor corrections:
a) Uses - defaults
Two issues here:
1. The entity sub-type link needs a link title (for accessibility) of "Entity module help", since that is where the link goes.
2. In the 2nd sentence, it says "manage fields page for entities", which is still not quite right... I think we should just say "On the Manage fields page for each entity sub-type, you can enable commenting...".
b) Uses - Overriding
This is not grammatically correct, and I think it doesn't quite get the point across. How about:
When you create an entity item, you can override the default comment settings. Change the entity sub-type defaults will not affect existing entity items, whether they used the default settings or had overrides.
The rest looks good, thanks! Almost there!
Comment #26
amitgoyal CreditAttribution: amitgoyal commentedSounds better! I have made the required changes mentioned in #25.
Please review.
Comment #27
jhodgdonNearly perfect! I found two typographical errors:
a) In Enabling:
That ; should not be there.
b) In Overriding:
Change ==> Changing
Rather than going through another round of patching, I simply edited the patch to fix these two spots (no other changes and I didn't make an interdiff).
So this is ready for a manual test (someone other than amitgoyal please!):
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.
Comment #28
mparker17All the links work.
Regarding matching terminology, the permission to administer comments is now called "Administer comments and comment settings". :S
I also noticed the following things which seem to be really minor: they might not be necessary to fix...
Formatting looks fine.
Once the permission to administer comments has been fixed, I'm ready to RTBC this.
Comment #29
jhodgdonThanks for testing! All of those things should be fixed, in my opinion. Regarding the comment settings order, that seems like a more logical order than what we have in the documentation.
Comment #30
amitgoyal CreditAttribution: amitgoyal commentedThanks @mparker17 and @jhodgdon for your feedback.
I have made all the changes as suggested in #28. Please check.
Comment #31
mparker17Looks good to me! Thanks for all your hard work @amitgoyal!
Comment #32
mparker17Also, unassigned now. :P
Comment #33
minneapolisdan CreditAttribution: minneapolisdan commentedManually tested patch, all seems good. RTBC.
Comment #34
jhodgdonGreat work everyone, and thanks again! Committed to 8.x.
Comment #36
andypostThis still tagged as needs manual testing, and as #2230177-20: Without Field UI comment module presents a poor UX states it's not fixed
Comment #37
nikita.izotov CreditAttribution: nikita.izotov commentedI wasn't able to revert the patch because issue is pretty old already and i just followed all conversation from up to the bottom, seems like everything is fixed and tested as @jhodgdon has mentioned in comment #34. I think that issue is not needed testing anymore.
Comment #38
jhodgdonThis issue is closed, and was closed a LONG time ago. Please open a new issue if there are additional things that need updating in the Comment module help.
Comment #39
andypostFollow-up filed #2488078: Update hook_help for Comment module about field UI need to manage comment fields