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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

berkas1’s picture

Taking care of this - during DC Prague sprint.

berkas1’s picture

Status: Active » Needs review
FileSize
3.82 KB

Status: Needs review » Needs work

The last submitted patch, comment-updated_help_d8-2091299-2.patch, failed testing.

berkas1’s picture

berkas1’s picture

Status: Needs work » Needs review
jover’s picture

Status: Needs review » Reviewed & tested by the community

The patch from #4 works.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

dbazuin’s picture

Assigned: Unassigned » dbazuin
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.21 KB

Done a rerole

dbazuin’s picture

Issue tags: +Needs reviewed

tagging

dbazuin’s picture

tagging

Status: Needs review » Needs work

The last submitted patch, comment-updated_help_d8-2091299-8.patch, failed testing.

berkas1’s picture

Status: Needs work » Needs review
FileSize
4.06 KB

path patched :D

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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.

InternetDevels’s picture

Fixed issues from previous comment.

jhodgdon’s picture

Looking 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.

jhodgdon’s picture

Status: Needs review » Needs work
jhodgdon’s picture

We 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.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
4.55 KB

@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.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
1.54 KB

@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.

jhodgdon’s picture

Status: Needs review » Needs work

The new wording looks much better, thanks!

A couple of things to fix:

a)

see the online documentation for the <a href="!comment">Comment module</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:

<a href='!content-type'>content type</a>

You may need to change the t() to use single quotes, and escape apostrophes like user\'s or whatever.

c)

Comment functionality can be enabled for any Drupal entity; e.g., a <a href='!content-type'>content type</a> and the behavior can be 

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.

Each entity can have its own default comment settings ...

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.

apply to all new content created ... settings on existing content must be done manually

I don't think I'd call it "content", it's "entity items"

3.

can be customized per content type and entity ...

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.

can be overridden for any given item of content

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)

When a comment has no replies, it remains editable by its author, as long as the author has 'Edit own comments' permission.

I do not think this belongs in the Configuring default and custom settings uses paragraph at all? Should be somewhere else.

f)

Setting up comment approval

This is not a good title for this Uses item. Maybe "Approving and managing comments"?

amitgoyal’s picture

Thanks 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.

jhodgdon’s picture

Status: Needs review » Needs work

Better, thanks!

Still some things to be fixed:

a) All links to drupal.org should be https

b)

'Comment functionality can be enabled for any Drupal <a href="!entity-help">entity</a>; (for example, a <a href="!content-type">content type</a>).

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:

The default settings can be made for each entity sub-type and overridden for each entity item. The entity sub-types each have their own default comment settings...

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)?

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
3.72 KB

@jhodgdon - I have tried to make changes as per your feedback in a, b, c & d. Please see if they look good to you.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good! A few very minor corrections:

a) Uses - defaults

Comment functionality can be enabled for any <a href="!entity-help">entity sub-type</a>; (for example, a <a href="!content-type">content type</a>). On the Manage fields page for entities (such as content items, taxonomy terms, etc.), you can enable commenting by adding a Comment field.

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

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.

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!

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
4.54 KB
3.03 KB

Sounds better! I have made the required changes mentioned in #25.

Please review.

jhodgdon’s picture

Nearly perfect! I found two typographical errors:

a) In Enabling:

...  title="Entity module help">entity sub-type</a>; (for example,  ...

That ; should not be there.

b) In Overriding:

... Change the entity sub-type defaults will not affect existing entity ...

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.

mparker17’s picture

Status: Needs review » Needs work

All 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...

  • The field type is actually named "Comments" (plural), not "Comment" (singular)
  • The default comment settings are in the order "Open", "Closed" and "Hidden" in the UI; the documentation explains them in the order "Open", "Hidden", "Closed"

Formatting looks fine.


Once the permission to administer comments has been fixed, I'm ready to RTBC this.

jhodgdon’s picture

Thanks 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.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
3.57 KB

Thanks @mparker17 and @jhodgdon for your feedback.

I have made all the changes as suggested in #28. Please check.

mparker17’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! Thanks for all your hard work @amitgoyal!

mparker17’s picture

Assigned: dbazuin » Unassigned

Also, unassigned now. :P

minneapolisdan’s picture

Issue tags: -Needs manual testing +RTBC

Manually tested patch, all seems good. RTBC.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBC +Needs manual testing

Great work everyone, and thanks again! Committed to 8.x.

Status: Fixed » Closed (fixed)

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

andypost’s picture

Status: Closed (fixed) » Needs work

This still tagged as needs manual testing, and as #2230177-20: Without Field UI comment module presents a poor UX states it's not fixed

nikita.izotov’s picture

I 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.

jhodgdon’s picture

Status: Needs work » Closed (fixed)
Issue tags: -Needs manual testing

This 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.

andypost’s picture