Problem/Motivation

#3041924: [META] Convert hook_help() module overview text to topics for the comment, node, path, taxonomy module(s).

Proposed resolution

Take the information that is currently in the hook_help module overview section for the module(s), and make sure the information is in one or more Twig help topic files. Steps:

  1. Find the hook_help() implementation function in the core/modules/MODULENAME/MODULENAME.module file(s). For example, for the core Contact module, the module files is core/modules/contact/contact.module, and the function is called contact_help().
  2. Locate the module overview portion of this function. This is located just after some lines that look something like this:
      switch ($route_name) {
        case 'help.page.contact':   /code>
    And ends either at the end of the function, or where you find another <code>case 'something':

    line.

  3. We want to end up with one or more topics about the tasks that you can do with this module, and possibly a section header topic. So, read the help and figure out a good way to logically divide it up into tasks and sections. See Standards for Help Topics for information on how to do this.
  4. See if some of these tasks are already documented in existing topics. Currently, all topics are in core/modules/help_topics/help_topics. Note that to see existing topics, you will need to enable the experimental Help Topics module (available in the latest dev versions of Drupal 8.x).
  5. For each task or section topic that needs to be written, make a new Twig topic file (see Standards for Help Topics) in core/modules/help_topics/help_topics. You will need to choose the appropriate module prefix for the file name -- the module that is required for the functionality. Alternatively, if the information spans several modules or if the information should be visible before the module is installed, you can use the "core" file name prefix. For instance, it might be useful to know that to get a certain functionality, you need to turn on a certain module (so that would be in the core prefix), but then the details of how to use it should only be visible once that module is turned on (so that would be in the module prefix).
  6. File names must be MODULENAME.TOPICNAME.html.twig -- for example, in the Action module, you could create a topic about managing actions with filename action.managing.html.twig (and "MODULENAME" can be "core" as discussed above).
  7. Make a patch file that adds/updates the Twig templates. The patch should not remove the text from the hook_help() implementation (that will be done separately).

Remaining tasks

a) Make a patch (see Proposed Resolution section).

b) Review the patch:

  1. Apply the patch.
  2. Turn on the experimental Help Topics module in your site, as well as the module(s) listed in this issue.
  3. Visit the page for each topic that is created or modified in this patch. The topics are files in the patch ending in .html.twig. If you find a file, such as core/modules/help_topics/help_topics/action.configuring.html.twig, you can view the topic at the URL admin/help/topic/action.configuring within your site.
  4. Review the topic text that you can see on the page, making sure of the following aspects:
    • The text is written in clear, simple, straightforward language
    • No grammar/punctuation errors
    • Valid HTML -- you can use http://validator.w3.org/ to check
    • Links within the text work
    • Instructions for tasks work
    • Adheres to Standards for Help Topics [for some aspects, you will need to look at the Twig file rather than the topic page].
  5. Read the old "module overview" topic(s) for the module(s), at admin/help/MODULENAME. Verify that all the tasks described in these overview pages are covered in the topics you reviewed.

User interface changes

Help topics will be added to cover tasks currently covered in modules' hook_help() implementations.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#123 3067727-123.patch40.12 KBjhodgdon
#123 interdiff-118-123.txt803 bytesjhodgdon
#122 Screenshot from 2021-04-24 13-40-44.png77.88 KBdaffie
#118 interdiff-115-118.txt2.72 KBjhodgdon
#118 3067727-118.patch40.12 KBjhodgdon
#115 interdiff-113-115.txt998 bytesshetpooja04
#115 3067727-115.patch40.13 KBshetpooja04
#113 interdiff-190-113.txt3.13 KBkishor_kolekar
#113 3067727-113.patch40.3 KBkishor_kolekar
#111 interdiff-109-111.txt2.2 KBshetpooja04
#111 3067727-111.patch40.31 KBshetpooja04
#109 3067727-109.patch40.38 KBandypost
#109 interdiff.txt38.51 KBandypost
#108 interdiff-105-108.txt2.4 KBjhodgdon
#108 3067727-108.patch38.65 KBjhodgdon
#105 interdiff-100-105.txt20.34 KBjhodgdon
#105 3067727-105.patch38.57 KBjhodgdon
#100 interdiff_94_100.txt4.24 KBanmolgoyal74
#100 3067727-100.patch38.76 KBanmolgoyal74
#94 3067727-94.patch38.69 KBjhodgdon
#94 interdiff-90-94.txt13.5 KBjhodgdon
#90 3067727-90.patch38.17 KBNitinLama
#90 interdiff_87-90.txt903 bytesNitinLama
#88 interdiff-84.txt6.29 KBPooja Ganjage
#87 3067727-87.patch38.1 KBNitinLama
#87 interdiff_78-87.txt6.42 KBNitinLama
#86 3067727-84.patch38.02 KBPooja Ganjage
#84 interdiff_78-84.txt6.42 KBNitinLama
#84 3067727-84.patch38.11 KBNitinLama
#81 3067727-81.patch38.16 KBNitinLama
#78 interdiff-78.txt16.66 KBPooja Ganjage
#78 3067727-78.patch38.15 KBPooja Ganjage
#74 3067727-74.patch38.36 KBPooja Ganjage
#72 3067727-72.patch38.35 KBjhodgdon
#72 interdiff-66-72.txt12.91 KBjhodgdon
#66 interdiff_63_66.txt3.29 KBanmolgoyal74
#66 3067727-66.patch38.33 KBanmolgoyal74
#63 interdiff.txt694 bytesjhodgdon
#63 3067727-63.patch38.26 KBjhodgdon
#61 3067727-61.patch38.26 KBjhodgdon
#59 interdiff_55-59.txt27.19 KBshetpooja04
#59 help-topic-3067727-59.patch38.28 KBshetpooja04
#55 interdiff_54-55.txt4.93 KBshetpooja04
#55 3067727-55.patch38.1 KBshetpooja04
#54 interdiff_51-54.txt17.97 KBshetpooja04
#54 3067727-54.patch38.09 KBshetpooja04
#51 3067727-50.patch35.52 KBbatigolix
#50 interdiff3.txt8.46 KBbatigolix
#49 3067727-49.patch35.64 KBbatigolix
#47 3067727-47.patch35.65 KBbatigolix
#47 interdiff2.txt2.97 KBbatigolix
#44 interdiff.txt2.65 KBbatigolix
#44 3067727-44.patch35.99 KBbatigolix
#41 3067727-41.patch36.16 KBbatigolix
#39 taxonomy-3067727-39.patch6.68 KBbatigolix
#38 path-3067727-38.patch5.39 KBbatigolix
#37 comment-3067727-37.patch11.27 KBbatigolix
#29 interdiff_24-29.txt31.84 KBnitesh624
#29 3067727-29.patch20.67 KBnitesh624
#15 664407_help_topic.patch18.49 KBGayathri J
#24 help-topic-3067727-24.patch14.89 KBmrinalini9
#8 3067727-hook-help-to-topics.patch18.47 KBiyyappan.govind
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kdassing created an issue. See original summary.

volkswagenchick’s picture

@kdassing created this issue in the first-time contributor workshop at DrupalCamp Asheville.

We reviewed the instructions from the meta ticket and created an issue to understand the process.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary with better instructions/guidelines

jhodgdon’s picture

Issue summary: View changes

And some even better guidelines now!

jhodgdon’s picture

Issue summary: View changes

We've migrated the help topic standards to https://www.drupal.org/docs/develop/documenting-your-project/help-topic-... so updating issue summary again.

Swapnil_Kotwal’s picture

Assigned: Unassigned » Swapnil_Kotwal
Swapnil_Kotwal’s picture

Assigned: Swapnil_Kotwal » Unassigned
iyyappan.govind’s picture

Hi Team & jhodgdon,

I have created the patch for help topics of comment, node, filter, path and taxonomy module. Please review it. I will do the corrections if any.
Thanks

iyyappan.govind’s picture

Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for making the patch... But please read the issue summary and standards for help topics that it links to. We are looking for task-based help, not copies of what is currently in the hook_help. There are instructions in the issue summary about how to make good topics.

iyyappan.govind’s picture

Hi @jhodgdon,

I will do it and re-patch it soon. Thanks

volkswagenchick’s picture

Issue tags: +badcamp2019

Tagging for badcamp2019, thanks! (October 2-5)

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jhodgdon’s picture

Issue summary: View changes

We just found out that all topic Twig files currently need to go into core/modules/help_topics/help_topics (with their finalized module-based file names), for the time being until the Help Topics module is stable. Updating issue summary. Patch will need to be updated too.

Gayathri J’s picture

I created patch for as per new help_topic standers please review. it require some more work i am going to work on this.

Status: Needs review » Needs work

The last submitted patch, 15: 664407_help_topic.patch, failed testing. View results

jhodgdon’s picture

Status: Needs work » Postponed

It looks like the patch has some errors.

I also recommend right now that we postpone this issue until #3087562: Convert datetime, datetime_range, field, field_ui, link, options, telephone, text modules hook_help() to topic(s) is done, because that module is providing a topic called "Managing content structure", which has a lot of useful background information for these topics.

jhodgdon’s picture

jhodgdon’s picture

Title: Convert comment, node, filter, path, taxonomy module hook_help() to topic(s) » Convert comment, node, path, taxonomy module hook_help() to topic(s)
Issue summary: View changes
Related issues: +#3067614: Convert filter, ckeditor, editor module hook_help() to topic(s)

Over on #3067614: Convert filter, ckeditor, editor module hook_help() to topic(s) around comment 40, we realized we probably needed to talk about text formats in conjunction with the editor modules. So, I've moved the filter module to that issue, and am removing it from here. We should maybe postpone this issue until that one is done too?

jhodgdon’s picture

The field/entity issue has been committed; this issue still needs to be postponed on the filter issue.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Status: Postponed » Needs work

Blocker commited #3067614: Convert filter, ckeditor, editor module hook_help() to topic(s)

Needs work for re-roll and newly added topics

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
14.89 KB

Rerolled patch for 9.1.x as per the newly added topics, please review.

Status: Needs review » Needs work

The last submitted patch, 24: help-topic-3067727-24.patch, failed testing. View results

jhodgdon’s picture

The test results indicate that there is a syntax problem in one of the topics, serious enough that it will not display and causes a 500 Internal error. So, the patch needs work.

A few general thoughts on making patches, and one thought on this specific patch:

a) It is a good idea to test your patch in a local development environment before submitting it -- clear the cache and you should be able to see any new topics you added in your patch.

b) Please make an interdiff file when you make a patch on an existing issue that previously had a patch. This lets reviewers see what you changed. If you made so many changes that the interdiff file is not useful (it's essentially the same as the whole patch), then you don't have to make an interdiff but you should state that in your comment when you upload the patch.

c) It is clear from a quick glance at this patch that you did not read the instructions in the issue summary or the help topic guidelines that are linked there. The patch seems to be a copy/paste from the hook_help, which is NOT what we are looking for. If you do not want to follow the instructions, please do not submit patches.

d) Actually, all of the previous patches on this issue suffer from the same problem. Probably whoever makes the next patch needs to start over and actually follow the instructions.

nitesh624’s picture

Assigned: Unassigned » nitesh624
iyyappan.govind’s picture

nitesh624’s picture

Changes done as per coment in #26. Please review thanks.

nitesh624’s picture

Assigned: nitesh624 » Unassigned
Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

The topics in this patch do not conform to the help topic standards (see link in the issue summary).

In a 3-minute glance, I found these problems -- but there are probably more:
a) Wrong verb tense in Goal sentence in all of the task-type topics. And some need more detail in the goal.
b) comment.overview.html.twig is just not following the standards at all
c) node.overview.html.twig -- should not use the word "node" except as the name of the module. We do not call content items "nodes" at all since Drupal 7.
d) Lots of grammatical problems.

batigolix’s picture

Assigned: Unassigned » batigolix

I'll will have a stab at this.

volkswagenchick’s picture

Issue tags: +Global2020

Tagging this for DrupalCon Global happening July 14-17

batigolix’s picture

As this is quite a big amount of topics, it may take some time before I can create a complete patch.
I have started with the comment module and will probably write the topics:
- commenting overview
- configuring content commenting
- Administering and approving comments

Maybe we can divide the work. If someone want to start with the node, taxonomy or path module, then go ahead.

Please leave a note in this issue.

jhodgdon’s picture

Yeah... good idea. Keep in mind we have some content already written about fields, content entities, text filters, and text editors, which came from the issues marked as Related to this issue. So that content should be referred to rather than duplicated.

jhodgdon’s picture

Issue tags: -Novice

Taking the Novice tag off this issue. It really isn't, at least until we get to the review stage.

batigolix’s picture

FileSize
11.27 KB

Here is the patch for the comment help_topics.

I will continue with path.
If someone want to do node or taxonomy, then go ahead.

Review can best wait, I think, until all modules from this issues have been done.

Interdiff doesnt make much sense as I have probably changed every line in the code.

batigolix’s picture

FileSize
5.39 KB

Here is the patch for the path help_topics.

I will continue with taxonomy.
If someone wants to do node, then go ahead.

Review can best wait, I think, until all modules from this issues have been done

batigolix’s picture

FileSize
6.68 KB

Here is the patch for the taxonomy help_topics.

I will continue with node.
If someone wants to do something, then go ahead.

Review can best wait, I think, until all modules from this issues have been done.

jhodgdon’s picture

I took a very quick look at the files above.

I think the list of topics looks pretty good in all of them, based on scanning the topic titles and the Goal sections on task topics.

A few things that I think will need to be fixed before we finalize this patch and commit it:

  • In all of the other help topics patches, we are not wrapping lines within individual P and LI tags.
  • Steps sections on task topics should launch right into an OL list of steps. If there is background information to be given, put it in a separate section with an H2 header ("What is...") or in an overview topic.
batigolix’s picture

Assigned: batigolix » Unassigned
Status: Needs work » Needs review
FileSize
36.16 KB

Find attached a patch that adds the help_topics for: comment, node, path and taxonomy.

It has been quite a copy-paste-modify job from various source, mainly the User Guide. I guess it will be visible here and there.

Please have a first look to see if the kind of topics are OK and whether important topics are missing.

The issues of #40 have been fixed.

Status: Needs review » Needs work

The last submitted patch, 41: 3067727-41.patch, failed testing. View results

jhodgdon’s picture

Thanks for the excellent patch! I took a quick look at the patch, and I think it looks good (haven't reviewed it in great detail yet). A few notes:

  1. The test fail -- It looks like there is a "related" item in topic taxonomy.overview that points to topic taxonomy.configuring_taxonomy that doesn't exist.
  2. The "Who can" sections on the Task topics seem like a good idea. We might think about adding that to some of the existing topics and the Help Topic Standards... In other topics, we've added that information to the overview topics but it wasn't on the task topics. Anyway, I added a note to the "cleanup" issue #3121340: Fix up minor copy problems in help topics.
  3. In the node.overview.html.twig topic in this patch, there's a section called "What is a content entity?" -- this section seems to repeat information that is in the core.content_structure topic. We should link to that topic instead of repeating the information.
  4. We are not using the term "node" in any UI in Drupal since Drupal 7:
    https://www.drupal.org/docs/develop/user-interface-standards/interface-t...
    So we should take that out. We can mention "Node module" as the name of the module, but we don't want to use the word "node" to refer to content item.
batigolix’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
35.99 KB
2.65 KB

Regarding: #43:

1. Is fixed. The reference can be removed because it is not necessary to have references in both topics.

2. Great!

3. I removed it and because there is no reference in the text, the reference in Related Topics is enough.

4. I removed the "What is a node" and added "What is the Node module" for which the old help topic contained a good description. So that is a literal copy&paste.

Please give it another look.

Status: Needs review » Needs work

The last submitted patch, 44: 3067727-44.patch, failed testing. View results

jhodgdon’s picture

Mostly great!

I'm actually not in favor of having "What is the xyz module?" topics or headers. What we've done in the other overview topics is in the last heading in the topic, we've had a heading called "Managing content overview" or something similar, and somewhere in there it says "This functionality is provided by the core Node module" or something similar.

It seems there is another test failure:

Topic path.overview Twig file has all of its text translated

That is because of this:

+<p>{% trans %}A path is the unique, last part of the URL for a specific function or piece of content. For example, for a page whose full URL is <em>http://example.com/node/7</em>, the path is <em>node/7</em>. Here are some examples of paths you might find in your site:{% endtrans %}</p>
+<ul>
+  <li>node/7</li>
+  <li>taxonomy/term/6</li>
+  <li>admin/content/comment</li>
+  <li>user/login</li>
+  <li>user/3</li>
+</ul>

The UL list is a list of paths, so you were probably correct to not translate it. However, we have a help topics test that makes sure all the text is translated, so that we don't forget to translate everything.

So... we will need to do one of two things:
a) Modify the test somehow to allow for some things to be untranslated
b) Modify the HTML here so that these are translated. For instance, each LI could say something like:

<li>{% trans %}node/7: Path to a particular content item{% endtrans %}</li>

(b) is going to be easier...

batigolix’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
35.65 KB

Ok, I think it might be better than without mentioning the Node module at all.

I fixed the missing trans tags in the bullet list, but took out some items.

Status: Needs review » Needs work

The last submitted patch, 47: 3067727-47.patch, failed testing. View results

batigolix’s picture

New attempt

batigolix’s picture

Status: Needs work » Needs review
FileSize
8.46 KB

New attempt. The interdiff shows changes since #44

batigolix’s picture

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for all the hard work on these topics!! They are all looking pretty good. I have a few suggestions (see also the standards page:
https://www.drupal.org/docs/develop/documenting-your-project/help-topic-...

a) Overall suggestions:
1. Try not to use the word Drupal in the help. Some people may use distributions or other means to obscure the fact they are running Drupal, and this is fine. Better wording can be "You can ___" or "Your site ___" or "The core software".
2. "is being shown" in several topics should be "is shown" (you've probably seen me comment on this in other reviews), when you arrive on a page and are describing what is shown there.
3. All overview/top-level topics should have, after the "What is..." sections, a section called something like "Overview of ____". This should list the common tasks and what core modules provide the functionality, and have a sentence saying "See the related topics listed below for specific tasks." (see item 4 under "Parts of a section topic" in the standards, or existing top-level topics for examples).
4. You should always "click" buttons, even drop-down buttons, not "choose" or "select" them.
5. All topic titles should start with a -ing verb, even top-level topics.
6. Where possible, link to other help topics rather than outside resources such as the user guide. For instance, we have a help topic about content entities (core.content_structure). It is also helpful to put links (with topic titles) in the text rather than simply adding them as related topics, to give context about why they are related. This is not necessary with all of the tasks related to an overview topic, since we'll have that sentence that says "See the related topics listed below for tasks".

b) The comment module topics:
1. You can actually enable comments on other types of entities now in Drupal 8/9, not just on nodes... so I am not sure we want to be that specific on comment.configuring_content ?
2. comment.configuring_content -- you could go straight to Manage Fields from the content types page (combine steps 2 and 3).
3. comment.configuring_content -- step 1 the link is going to the "comment types" page, not the "content types" page.
4. comment.configuring_content -- this topic seems to assume that the content type already has comments enabled? If you wanted to enable commenting on a content type that didn't already have comments enabled, you would need to follow different steps.
5. comment.creating_type -- seems to confuse content types with comment types?
6. comment.moderating -- the navigation in step 1 can just say to navigate to Content > Comments rather than talking about tabs.

c) The node module topics:
1. I think we need to be careful not to imply that nodes (which of course we call content items) are the only content entities on the site. So for instance, on the overview topic, it says "A content item is any piece of individual content" but I think this is misleading, because taxonomy terms and custom blocks are also "pieces of content".

d) the path module topics:
1. It might be useful to provide documentation on how to add an alias for a system path, taxonomy term, etc. The topics there only provide documentation on how to add an alias for a content item.

e) Taxonomy topics:
1. As in comments, you can actually use taxonomy as a field on other entities, such as custom blocks.
2. I am not sure we need to provide full steps for how to add fields. Instead, we could link to the topic on how to add reference fields.

batigolix’s picture

Assigned: Unassigned » batigolix

I am working on a new patch

shetpooja04’s picture

Status: Needs work » Needs review
FileSize
38.09 KB
17.97 KB

I am working on this issue in parts.
I have tried to implement the changes and added patch for the point a) Overall suggestions: mentioned in #52
Please review
Thank you!

Below points are still remaining:

b) The comment module topics:
1. You can actually enable comments on other types of entities now in Drupal 8/9, not just on nodes... so I am not sure we want to be that specific on comment.configuring_content ?
2. comment.configuring_content -- you could go straight to Manage Fields from the content types page (combine steps 2 and 3).
3. comment.configuring_content -- step 1 the link is going to the "comment types" page, not the "content types" page.
4. comment.configuring_content -- this topic seems to assume that the content type already has comments enabled? If you wanted to enable commenting on a content type that didn't already have comments enabled, you would need to follow different steps.
5. comment.creating_type -- seems to confuse content types with comment types?
6. comment.moderating -- the navigation in step 1 can just say to navigate to Content > Comments rather than talking about tabs.

c) The node module topics:
1. I think we need to be careful not to imply that nodes (which of course we call content items) are the only content entities on the site. So for instance, on the overview topic, it says "A content item is any piece of individual content" but I think this is misleading, because taxonomy terms and custom blocks are also "pieces of content".

d) the path module topics:
1. It might be useful to provide documentation on how to add an alias for a system path, taxonomy term, etc. The topics there only provide documentation on how to add an alias for a content item.

e) Taxonomy topics:
1. As in comments, you can actually use taxonomy as a field on other entities, such as custom blocks.
2. I am not sure we need to provide full steps for how to add fields. Instead, we could link to the topic on how to add reference fields.

shetpooja04’s picture

I am working on this issue in parts.
I have tried to implement the changes and added patch for the point b) The comment module topics: mentioned in #52
Please review.
Thank you!

Below points are still remaining:

c) The node module topics:
1. I think we need to be careful not to imply that nodes (which of course we call content items) are the only content entities on the site. So for instance, on the overview topic, it says "A content item is any piece of individual content" but I think this is misleading, because taxonomy terms and custom blocks are also "pieces of content".

d) the path module topics:
1. It might be useful to provide documentation on how to add an alias for a system path, taxonomy term, etc. The topics there only provide documentation on how to add an alias for a content item.

e) Taxonomy topics:
1. As in comments, you can actually use taxonomy as a field on other entities, such as custom blocks.
2. I am not sure we need to provide full steps for how to add fields. Instead, we could link to the topic on how to add reference fields.

batigolix’s picture

Assigned: batigolix » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patches! I have looked at the patch in #55 and compared to the review points I made in #54. Still some things to fix:

Overall: The topics need proofreading for grammar and punctuation. For example, some sentences don't end in . Until the content is closer to correct, though, I have not made specific proofreading suggestions.

a.1) The word "Drupal" still appears in path.overview.html.twig

a.2) "being shown" still appears twice in comment.configuring_content.html.twig

a.3) Overview sections were added... In comment.overview.html.twig the grammar here is not great:
This module allows the below listed things -- And also we are trying very hard NOT to make topics about *modules* but instead about *tasks*. So "this module" is very confusing -- it doesn't even say which module is being discussed. It should say something like:

The core Comment module provides the following functionality:

node.overview.html.twig and path.overview.html.twig and taxonomy.overview.html.twig have the same problem.

a.4) Terminology on click, select, etc... here is some guidance from the User Guide guidelines:

When providing instructions for interacting with the user interface, use the following styles. For clicking on a link or button, use the phrase "Click (link or button text)", rather than "click on", "select", or other terms. For a select list or radio button, say "Select (choice)". For a checkbox, say "Check (choice)". For a drop-down list of operations links, say "Click (choice)". Make sure that the actual UI text you are selecting or clicking on is in italics.

Example of where this usage is incorrect, in the taxonomy.configuring topic:
...In the <em>Vocabulary</em> field, choose the vocabulary that you created.
(should be "select" not "choose").

I have not looked through the entire patch to verify other usage, but I expect there may be other places where this is incorrect.

a.5) Topic titles:
Creating custom comment type ==> Creating a custom comment type
Configuring paths, aliases and URLs ==> Configuring paths, aliases, and URLs
[Note on that one: The Drupal project style guide says to use the "Oxford comma", so you need a comma before "and"]

b.1 -- This is still an issue. The problem is that you can actually enable comments on taxonomy terms, content items, custom blocks, user profiles, etc. But the topic is specific to content items. That still needs to be fixed.

c.1 -- this is still an issue. The node.overview topic says:

+<h2>{% trans %}What is a content item?{% endtrans %}</h2>
+<p>{% trans %}A <em>content item</em> is any piece of individual content, such as a page, poll, article, forum topic, or a blog entry.{% endtrans %}</p>

This is just not true. Custom blocks are also "pieces of individual content", but they are not "content items". I think we have some wording from the core.content_structure topic (already in the repo) that can be adopted.

d.1 -- this is still an issue. path.creating_alias.html.twig is called "Creating a URL alias", but the instructions are about specifically creating a URL alias for a node module content item.

e -- taxonomy... New note:
e.3 -- The configuring topic refers to "create a taxonomy". That is not the right terminology. It is called a "vocabulary" not a "taxonomy".
The other two points, e.1 and e.2 still are problems too.

shetpooja04’s picture

Thank you for reviewing the patch and your feedback.
I am working on it.

shetpooja04’s picture

Status: Needs work » Needs review
FileSize
38.28 KB
27.19 KB

I have made change according to the suggestions mentioned in #57

Remaining points

From #52
d) the path module topics:
1. It might be useful to provide documentation on how to add an alias for a system path, taxonomy term, etc. The topics there only provide documentation on how to add an alias for a content item.

e) Taxonomy topics:
1. As in comments, you can actually use taxonomy as a field on other entities, such as custom blocks.
2. I am not sure we need to provide full steps for how to add fields. Instead, we could link to the topic on how to add reference fields.

From #57
d.1 -- this is still an issue. path.creating_alias.html.twig is called "Creating a URL alias", but the instructions are about specifically creating a URL alias for a node module content item.

e -- taxonomy... New note:
e.3 -- The configuring topic refers to "create a taxonomy". That is not the right terminology. It is called a "vocabulary" not a "taxonomy".
The other two points, e.1 and e.2 still are problems too.

jhodgdon’s picture

Thanks for the new patch! I am in the process of reviewing it, and as there have been a lot of patches and still there are still questions, I've decided the most efficient way for me to review is actually to make a new patch and interdiff. So, I'm working on that. There are a lot of topics in this patch and I'm reviewing/editing carefully, so it may take me a few days.

jhodgdon’s picture

Here's a new patch. Nearly every line in the patch changed at least a little bit, so an interdiff is really not useful.

I haven't tested this patch, so there could be some syntax errors. A thorough test and review would be much appreciated! There are review instructions in the issue summary.

Status: Needs review » Needs work

The last submitted patch, 61: 3067727-61.patch, failed testing. View results

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
38.26 KB
694 bytes

fix fail hopefully.

Status: Needs review » Needs work

The last submitted patch, 63: 3067727-63.patch, failed testing. View results

jhodgdon’s picture

OK, I'll need to fire up my other machine where I can actually run the tests and fix these problems. Sorry about the noise! I'll make time for this today.

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
38.33 KB
3.29 KB

Fixed the syntax error in modules/help_topics/help_topics/node.editing.html.twig

jhodgdon’s picture

Thanks for the new patch! The syntax tests pass locally for me now, and I think the patch is now ready for a complete review/test.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

tanubansal’s picture

Tested #66, its working fine on 9.1
This can be moved to RTBC

jhodgdon’s picture

Thanks for testing! Can you confirm that you reviewed the patch fully? There are review guidelines in the issue summary.

daffie’s picture

Status: Needs review » Needs work

I am not a native English speaker, therefore feel free to disagree.

  1. +++ b/core/modules/help_topics/help_topics/comment.overview.html.twig
    @@ -0,0 +1,29 @@
    +<p>{% trans %}The core software allows website visitors to post comments to discuss content like forum topics, blog posts, and news articles. Comments are a type of content entity, and can have fields that store text, HTML markup, and other data. Comments are attached to other content entities via Comment fields. See <a href="{{ content_structure }}">Managing content structure<a> for more about content entities and fields.{% endtrans %}</p>
    

    Could we start the first sentence with "A comment is ..." instead of "The core software ..."

  2. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,38 @@
    +      <li>{% trans %}<em>Anonymous commenting</em>: If anonymous users have permission to post comments, whether they are allowed or required to leave contact information with their comments.{% endtrans %}</li>
    

    The sentence does not read so good. "If ..., whether ...".

  3. Nitpick: In comment.overview in the steps part after the ":" part: sometimes start with an uppercase character and sometimes a lowercase character.
  4. +++ b/core/modules/help_topics/help_topics/comment.creating_type.html.twig
    @@ -0,0 +1,26 @@
    +{% set permissions = render_var(url('user.admin_permissions', {},{'fragment': 'module-comment'})) %}
    
    +++ b/core/modules/help_topics/help_topics/comment.disabling.html.twig
    @@ -0,0 +1,25 @@
    +{% set comment_permission_url = render_var(url('user.admin_permissions', {},{'fragment': 'module-comment'})) %}
    
    +++ b/core/modules/help_topics/help_topics/node.creating_content.html.twig
    @@ -0,0 +1,30 @@
    +{% set permissions = render_var(url('user.admin_permissions', {},{'fragment': 'module-node'})) %}
    
    +++ b/core/modules/help_topics/help_topics/node.creating_type.html.twig
    @@ -0,0 +1,30 @@
    +{% set permissions = render_var(url('user.admin_permissions', {},{'fragment': 'module-node'})) %}
    
    +++ b/core/modules/help_topics/help_topics/node.editing.html.twig
    @@ -0,0 +1,33 @@
    +{% set permissions = render_var(url('user.admin_permissions', {},{'fragment': 'module-node'})) %}
    
    +++ b/core/modules/help_topics/help_topics/taxonomy.configuring.html.twig
    @@ -0,0 +1,30 @@
    +{% set taxonomy_permissions = render_var(url('user.admin_permissions', {},{'fragment': 'module-taxonomy'})) %}
    
    +++ b/core/modules/help_topics/help_topics/taxonomy.overview.html.twig
    @@ -0,0 +1,27 @@
    +{% set taxonomy_permissions = render_var(url('user.admin_permissions', {},{'fragment': 'module-taxonomy'})) %}
    

    Nitpick: Needs a space between "{}," and "{'fragment"

  5. +++ b/core/modules/help_topics/help_topics/comment.disabling.html.twig
    @@ -0,0 +1,25 @@
    +      <li>{% trans %}<em>Open</em>: comments are allowed{% endtrans %}</li>
    +      <li>{% trans %}<em>Closed</em>: no new comments are allowed, but any past comments remain visible.{% endtrans %}</li>
    +      <li>{% trans %}<em>Hidden</em>: no comments are allowed, and past comments are hidden.{% endtrans %}</li>
    

    Nitpick: Do the sentences end with a dot or not?

  6. +++ b/core/modules/help_topics/help_topics/node.editing.html.twig
    @@ -0,0 +1,33 @@
    +<p>{% trans %}Find a content item and edit it, or update a group of content itesms in bulk. See <a href="{{ overview }}">Managing content</a> for more about content types and content items.{% endtrans %}</p>
    

    Misspelled "itesms".

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
12.91 KB
38.35 KB

Thank you very much for the careful review! I agreed with all of your complaints; I may have resolved them slightly differently than what you suggested, but hopefully we are iterating towards better topics! Here's an interdiff and a new patch.

As a note, before setting this issue to RTBC, someone needs to go through the review steps in the issue summary. Thanks!!

daffie’s picture

Status: Needs review » Needs work

I am bit out of time. I will review more later.

  1. +++ b/core/modules/help_topics/help_topics/taxonomy.overview.html.twig
    @@ -0,0 +1,27 @@
    +{% set taxonomy_permissions = render_var(url('user.admin_permissions', {}, {'fragment': 'module-taxonomy'})) %}
    +{% set taxonomy_admin = render_var(url('entity.taxonomy_vocabulary.collection')) %}
    +{% set entity_reference = render_var(url('help.help_topic', {'id':'field_ui.reference_field'})) %}
    

    These 3 links are not used.

  2. +++ b/core/modules/help_topics/help_topics/taxonomy.configuring.html.twig
    @@ -0,0 +1,30 @@
    +{% set taxonomy_permissions = render_var(url('user.admin_permissions', {}, {'fragment': 'module-taxonomy'})) %}
    ...
    +<p>{% trans %}Users with the <em><a href="{{ permissions }}">Administer vocabularies and terms</a></em> permission can configure a vocabulary. To add a field using the Field UI module, you will also need the <em>Administer fields</em> permission for the entity you are adding the field to.{% endtrans %}</p>
    

    Variables "taxonomy_permissions" and "permissions" are not the same.

  3. +++ b/core/modules/help_topics/help_topics/node.overview.html.twig
    @@ -0,0 +1,19 @@
    +<p>{% trans %}A <em>content item</em> is a type of content entity for page-level content, which can have fields that store text, HTML markup, images, attached files, and other data. See <a href="{{ content_structure }}">Managing content structure<a> for more about content entities and fields.{% endtrans %}</p>
    

    The closing tag <a> should be changed to </a>.

  4. +++ b/core/modules/help_topics/help_topics/node.editing.html.twig
    @@ -0,0 +1,33 @@
    +<p>{% trans %}Find a content item and edit it, or update a group of content items in bulk. See <a href="{{ overview }}">Managing content</a> for more about content types and content items.{% endtrans %}</p>
    

    The link "overview" does not exist.

  5. +++ b/core/modules/help_topics/help_topics/path.editing_alias.html.twig
    @@ -0,0 +1,20 @@
    +{% set permission = render_var(url('user.admin_permissions', {}, {'fragment': 'module-path'})) %}
    ...
    +<p>{% trans %}Users with the <em><a href="{{ permissions }}">Administer URL aliases</a></em> permission can edit aliases.{% endtrans %}</p>
    

    The link does not work correctly. "permission" is not equal to "permissions".

  6. +++ b/core/modules/help_topics/help_topics/comment.overview.html.twig
    @@ -0,0 +1,29 @@
    +<p>{% trans %}A comment is a piece of content, typically posted by a website visitor, which provides discussion or commentary on other content like forum topics, blog posts, and news articles. Comments are a type of content entity, and can have fields that store text, HTML markup, and other data. Comments are attached to other content entities via Comment fields. See <a href="{{ content_structure }}">Managing content structure<a> for more about content entities and fields.{% endtrans %}</p>
    

    The link Managing content structure is not closed correctly <a> instead of </a>.

  7. +++ b/core/modules/help_topics/help_topics/comment.creating_type.html.twig
    @@ -0,0 +1,26 @@
    +  <li>{% trans %}In the <em>Target entity type</em> field, select the entity type to be commented on. See <a href="{{ content_structure }}">Managing content structure<a> for more about content entities and fields.{% endtrans %}</li>
    

    The link Managing content structure is not closed correctly <a> instead of </a>.

  8. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,38 @@
    +<p>{% trans %}In order to follow these steps, the core Field UI module will need to be installed. You'll need the Comment module's <em><a href="{{ comment_permission_url }}">Administer comments and comment settings</a></em> permission, to change comment settings for a comment field. You'll also need to have the appropriate permission for adding fields to the entity type or subtype that the comments are attached to. For example, to attach comments to content items provided by the Node module, you would need the Node module's <em>Administer content types</em> permission.{% endtrans %}</p>
    

    The link Managing content structure is not closed correctly <a> instead of </a>.

  9. +++ b/core/modules/help_topics/help_topics/comment.disabling.html.twig
    @@ -0,0 +1,25 @@
    +<p>{% trans %}Turn off commenting for a particular content entity. See <a href="{{ content_structure }}">Managing content structure<a> for more about content entities and fields. Note that if you want to turn off commenting for all entities of an entity type or subtype, you will need to edit the field settings for the comment field; see the <a href="{{ comment_config }}">Configuring content commenting</a> topic for more about the comment field.{% endtrans %}</p>
    

    The link Managing content structure is not closed correctly <a> instead of </a>.

Pooja Ganjage’s picture

Hi,

Creating a patch as suggested in #73 comment but excepted point 1st and 4th.

Please review the patch.

Thanks.

daffie’s picture

@Pooja, could you also post an interdiff file. See: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

jhodgdon’s picture

Also, it looks like from your comment that you didn't know how to deal with points 1 and 4 from the review?

For point 1, you can delete the lines that define variables that are not being used.

For point 4, a variable needs to be added that points to the topic whose title is "Managing content". You can copy the line from the node.creating_content.html.twig topic.

jhodgdon’s picture

Also @daffie thanks very very much for reviewing!!

Pooja Ganjage’s picture

Hi,

Attached Updated patch and interdiff as suggested in #73 and #77 comment.

Please review the patch and interdiff.

Thanks

Pooja Ganjage’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/help_topics/help_topics/path.creating_alias.html.twig
    @@ -0,0 +1,26 @@
    +{% set path_permission = render_var(url('user.admin_permissions', {}, {'fragment': 'module-path'})) %}
    ...
    +<p>{% trans %}Users with the <em><a href="{{ permissions }}">Create and edit URL aliases</a></em> permission can create aliases. To follow the steps in this topic, you will also need permission to edit the content item.{% endtrans %}</p>
    

    These link names are not the same "path_permission" and "permissions".

  2. +++ b/core/modules/help_topics/help_topics/path.creating_alias.html.twig
    @@ -0,0 +1,26 @@
    +{% set path_aliases = render_var(url('entity.path_alias.collection')) %}
    

    This link is not used.

  3. +++ b/core/modules/help_topics/help_topics/path.editing_alias.html.twig
    @@ -0,0 +1,20 @@
    +{% set path_aliases = render_var(url('entity.path_alias.collection')) %}
    

    This link is not used.

  4. +++ b/core/modules/help_topics/help_topics/node.editing.html.twig
    @@ -0,0 +1,34 @@
    +      <li>{% trans %}<em>Published status</em>{% endtrans %}
    

    Line is missing </li>.

  5. +++ b/core/modules/help_topics/help_topics/node.editing.html.twig
    @@ -0,0 +1,34 @@
    +      <li>{% trans %}<em>Language</em>{% endtrans %}
    +      </li>
    

    Could we put this on a single line.

  6. +++ b/core/modules/help_topics/help_topics/comment.moderating.html.twig
    @@ -0,0 +1,27 @@
    +{% set comment_types = render_var(url('entity.comment_type.collection')) %}
    +{% set content = render_var(url('system.admin_content')) %}
    

    These links are not used.

  7. +++ b/core/modules/help_topics/help_topics/comment.disabling.html.twig
    @@ -0,0 +1,25 @@
    +{% set content = render_var(url('system.admin_content')) %}
    ...
    +<p>{% trans %}Turn off commenting for a particular content entity. See <a href="{{ content_structure }}">Managing content structure</a> for more about content entities and fields. Note that if you want to turn off commenting for all entities of an entity type or subtype, you will need to edit the field settings for the comment field; see the <a href="{{ comment_config }}">Configuring content commenting</a> topic for more about the comment field.{% endtrans %}</p>
    

    This link does not work.

  8. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,38 @@
    +<p>{% trans %}In order to follow these steps, the core Field UI module will need to be installed. You'll need the Comment module's <em><a href="{{ comment_permission_url }}">Administer comments and comment settings</a></em> permission, to change comment settings for a comment field. You'll also need to have the appropriate permission for adding fields to the entity type or subtype that the comments are attached to. For example, to attach comments to content items provided by the Node module, you would need the Node module's <em>Administer content types</em> permission.{% endtrans %}</p>
    

    This link does not work.

NitinLama’s picture

Assigned: Unassigned » NitinLama
Status: Needs work » Needs review
FileSize
38.16 KB

As per #80, attaching updated patch.

daffie’s picture

@NitinLama: Could you also add an interdiff.txt file. It makes reviewing the changes that you made to the patch file a lot easier. See: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

daffie’s picture

Status: Needs review » Needs work
+++ b/core/modules/help_topics/help_topics/comment.disabling.html.twig
@@ -0,0 +1,26 @@
+{% set content = render_var(url('system.admin_content')) %}

This link is now longer used, it can be removed.

NitinLama’s picture

Status: Needs work » Needs review
FileSize
38.11 KB
6.42 KB

As per #82 #83, sorry for the delay @daffie

daffie’s picture

Status: Needs review » Needs work
+++ b/core/modules/help_topics/help_topics/path.creating_alias.html.twig
@@ -0,0 +1,27 @@
+nitin

I think that this is a mistake.

Pooja Ganjage’s picture

Hi,

Creating a patch as suggested in #80 and #83 comment.

Please review the patch.

Thanks.

NitinLama’s picture

Status: Needs work » Needs review
FileSize
6.42 KB
38.1 KB

@daffie updated the patch.

Pooja Ganjage’s picture

FileSize
6.29 KB

Attached interdiff for #86 comment.

daffie’s picture

Status: Needs review » Needs work

I reviewed the patch from @nitinlama, because he assigned the issue to himself. Sorry Pooja Ganjage.

The patch from comment #87 look good to me with one exception:

+++ b/core/modules/help_topics/help_topics/path.editing_alias.html.twig
@@ -4,7 +4,6 @@ related:
-{% set path_aliases = render_var(url('entity.path_alias.collection')) %}

I made a mistake by asking to remove this link. The link is needed, so could you please put it back.

NitinLama’s picture

Status: Needs work » Needs review
FileSize
903 bytes
38.17 KB

here @daffie

NitinLama’s picture

Assigned: NitinLama » Unassigned
daffie’s picture

Status: Needs review » Reviewed & tested by the community

I followed the review process as described in the IS.
It all looks good to me.
For me it is RTBC.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for all of the reviews and patches, everyone!

I took one more look at the topics in the patch, and I think that are looking really good. However, I found a few things that I think need another look:

1. comment.configuring.html.twig

+  <li>{% trans %}Follow the steps in the related <em>Adding a field to an entity sub-type</em> topic to add a field of type <em>Comment</em> to the desired entity type or sub-type.{% endtrans %}</li>
+  <li>{% trans %}Click <em>Manage fields</em> from the dropdown button for the content type where you want to enable commenting.{% endtrans %}</li>
+  <li>{% trans %}When you get to the field settings page, enter the desired settings for the comment field:{% endtrans %}

These steps don't make sense together. The first step says to read the other topic, which already has the bit about the Manage Fields page. The second step here should simply be deleted. Then the third step makes sense, because the "adding a field" topic gets you to the field settings page.

2. In comment.creating_type.html.twig, it talks about creating a comment type. But in comment.configuring.html.twig, it doesn't mention comment types. I think there must be a setting on the Comment field to select a particular comment type to use? That needs to be added to comment.configuring.html.twig. Probably the setting only appears if you have more than one comment type, so that should be mentioned with a link to the comment.creating_type.html.twig topic.

At least, I think you can create more than one comment type for a given entity type? I'm not exactly sure about that... If you cannot, then that should be mentioned in the comment.creating_type.html.twig topic, and we wouldn't need to mention the comment type setting in the comment.creating_type.html.twig topic.

In either case, I think in comment.creating_type.html.twig we need to say that you can't configure commenting for an entity type unless a comment type already exists for that entity type (with a link to the creating_type topic)?

3. path.creating_alias.html.twig

+  <li>{% trans %}Locate and open the content editing form for the content item, or create a new content item (see related topics on managing and creating content).{% endtrans %}</li>

The two node topics that are related are node.creating_content and node.editing. So I think this should say "see related topics on creating and editing content".

4. path.creating_alias.html.twig -- This topic is specific to creating an alias for a node content item... Should we mention that you can do something similar for other entity types that are displayed on their own pages (such as taxonomy terms)?

5. path.editing_alias.html.twig -- nitpick: in the first step, navigation -- is "Search and Metadata" the same capitalization as what you actually see in the administrative UI? I think maybe metadata is not capitalized?

6. taxonomy.configuring.html.twig -- in the similar topic in the comment module, in the "who can" section, we explicitly mention that the Field UI module needs to be installed. We should do the same here.

7. taxonomy.overview.html.twig -- nitpick -- "Taxonomy" should not be capitalized in the first heading "What is taxonomy?".

jhodgdon’s picture

Here's a new patch, which addresses the previous review that I made. Some notes:

Item 1. fixed. Also noticed that the field_ui topic we're referencing talks about setting the "Allowed number of values" setting for fields, but this is disabled for Comments fields, so I documented that also. Also noticed that the field type is called "Comments" not "Comment" in the UI, while I was testing, so I fixed that also.

Item 2. I investigated and you can definitely have more than one comment type per entity type, so I adjusted the comment.configuring topic to be explicit about comment types.

Item 3. fixed.

Item 4. Actually this was already mentioned in the Goal section of this topic, so no change needed.

Item 5. I was correct and that has been fixed.

Item 6. Fixed.

Item 7. Fixed.

Other: Also fixed a typo on the comment type topic where it said "content type" instead of "comment type" in one of the headers. And in comment.overview, revised the section on comment types slightly to give I think a better example.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All the changes look good to me.
Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 94: 3067727-94.patch, failed testing. View results

daffie’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC.

jhodgdon’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work

Not quite a full review yet, but found a few things:

  1. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,40 @@
    +---
    +label: 'Configuring content commenting'
    +related:
    

    Why not 'Configuring comments'?

  2. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,40 @@
    +<h2>{% trans %}Who can configure comments?{% endtrans %}</h2>
    +<p>{% trans %}In order to follow these steps, the core Field UI module will need to be installed. You'll need the Comment module's <em><a href="{{ comment_permission_url }}">Administer comments and comment settings</a></em> permission, to change comment settings for a comment field. You'll also need to have the appropriate permission for adding fields to the entity type or subtype that the comments are attached to. For example, to attach comments to content items provided by the Node module, you would need the Node module's <em>Administer content types</em> permission.{% endtrans %}</p>
    +<h2>{% trans %}Steps{% endtrans %}</h2>
    

    Can we make this less passive, i.e. Ensure that the Field UI module is installed'?

    Also 'to attach comments' seems like it could either mean 'add a comment field to' or 'post comments on'.

  3. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,40 @@
    +    <ul>
    +      <li>{% trans %}<em>Threading</em>: whether or not the comments are collected by threads, with people able to reply to particular comments instead of to the content entity itself. (Note that not all themes support the display of comment threads with indentation.){% endtrans %}</li>
    +      <li>{% trans %}<em>Comments per page</em>: the maximum number of comments displayed on one page (additional pages will be added if you exceed this limit).{% endtrans %}</li>
    +      <li>{% trans %}<em>Anonymous commenting</em>: whether or not anonymous commenters are allowed or required to leave contact information with their comments (only applies if anonymous users have permission to post comments).{% endtrans %}</li>
    

    Instead of 'additional pages will be added', what about 'a pager will be added'?

  4. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,40 @@
    +        </ul>
    

    'no new comments' could be used in both sentences for consistency I think.

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
38.76 KB
4.24 KB

Addressed #99

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Back there, thank you @anmolgoyal74!

catch’s picture

Another partial review, but found some more issues.

  1. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,40 @@
    +<h2>{% trans %}Who can configure comments?{% endtrans %}</h2>
    +<p>{% trans %}In order to follow these steps, tEnsure that the Field UI module is installed. You'll need the Comment module's <em><a href="{{ comment_permission_url }}">Administer comments and comment settings</a></em> permission, to change comment settings for a comment field. You'll also need to have the appropriate permission for adding fields to the entity type or subtype that the comments are attached to. For example, add a comment field to content items provided by the Node module, you would need the Node module's <em>Administer content types</em> permission.{% endtrans %}</p>
    +<h2>{% trans %}Steps{% endtrans %}</h2>
    

    Typo: tEnsure

  2. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,40 @@
    +    <ul>
    +      <li>{% trans %}<em>Threading</em>: whether or not the comments are collected by threads, with people able to reply to particular comments instead of to the content entity itself. (Note that not all themes support the display of comment threads with indentation.){% endtrans %}</li>
    +      <li>{% trans %}<em>Comments per page</em>: the maximum number of comments displayed on one page (a pager will be added if you exceed this limit).{% endtrans %}</li>
    

    I'm not sure we need to say this about not all theme - feels like we're mentioning a bug with themes, when themes can have bugs (or just decide not to indent threads).

  3. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -0,0 +1,40 @@
    +        <ul>
    +          <li>{% trans %}<em>Open</em>: comments are allowed.{% endtrans %}</li>
    +          <li>{% trans %}<em>Closed</em>: no new comments are allowed, but any past comments remain visible.{% endtrans %}</li>
    +          <li>{% trans %}<em>Hidden</em>: no new comments are allowed, and past comments are hidden.{% endtrans %}</li>
    

    It would be good if we could bring the differing information to the beginning of the latter sentences.

    "Closed: Past comments remain visible but no new comments are allowed."

    "Hidden: past comments are hidden and no new comments are allowed." maybe?

Also reading this I think we need to open an issue to discuss whether we can tie comment moderation to the content moderation module somehow. (edit: here it is #3202259: Replace comment 'moderation' with content_moderation).

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the review! Status update.

jhodgdon’s picture

I'm working on a new patch...

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
38.57 KB
20.34 KB

I have carefully reviewed all of the topics in this patch. Besides what @catch found in #102, I found a few other places where I thought the wording could be improved or made more consistent with other topics. In those topics, I also took care of #3192585: Fix up topics to use new help_topic_link function since I was editing them anyway, but I didn't do that for all of the topics since we have another issue to do it.

So, here's a new patch. The topics that are in the interdiff could use a careful review/test as outlined in the issue summary.

Status: Needs review » Needs work

The last submitted patch, 105: 3067727-105.patch, failed testing. View results

jhodgdon’s picture

Looks like I made a syntax error in one of the topics. I'll have to fire up my dev machine and see what's up.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
38.65 KB
2.4 KB

Looks like I missed a % in one place, and then copy/pasted the error into 2 other topics.

andypost’s picture

FileSize
38.51 KB
40.38 KB

Finished converting to new URL-twig functions (started in #105) https://www.drupal.org/node/3192582

Also consistently naming links and topics, also ordered - links first and topics last variables

jhodgdon’s picture

Status: Needs review » Needs work

The interdiff looks mostly good, thanks! This bit is not right though:

--- a/core/modules/help_topics/help_topics/node.creating_content.html.twig
+++ b/core/modules/help_topics/help_topics/node.creating_content.html.twig
...
+{% set content_add_link_text %}
+  {% trans %}Article: Create new content{% endtrans %}
+{% endset %}
+{% set content_add_link = render_var(help_route_link(content_add_link_text, 'user.admin_permissions', {}, {'fragment': 'module-node'})) %}
...
+  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>{{ content_add_link }}</em>.{% endtrans %}</li>

This links to the permissions page, but it should link to the Content overview page.

Also I don't think we should be substituting in the create new article permission here:

...
For example, to create content of type Article, a user would need the <em>{{ content_add_link_text }}</em> permission.
shetpooja04’s picture

Status: Needs work » Needs review
FileSize
40.31 KB
2.2 KB

Hey, I have tried to cover points mentioned in #110
Please review

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for trying!

I guess I wasn't clear in #110 and in Slack, sorry! There are still two problems in the node.creating_content.html.twig topic:

a) In this sentence:

 For example, to create content of type Article, a user would need the permission.

I still think we need to say which permission someone would need here -- the sentence doesn't read correctly. It should say a user would need the Article: Create new content permission.

b) This part of the patch is also still not right:

+{% set content_add_link_text %}
+  {% trans %}Article: Create new content{% endtrans %}
+{% endset %}
+{% set content_add_link = render_var(help_route_link(content_add_link_text, 'system.admin_content')) %}
...
+  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>{{ content_add_link }}</em>.{% endtrans %}</li>

Problems with this:
- It's a link to admin/content but the link text says "Article: create new content". The link text should be "Content".
- The variable name is confusing. It's a link to the admin/content page. The variable name should probably be content_link not content_add_link.

I suggest going back to the patch in #109 and trying again from there.

kishor_kolekar’s picture

Status: Needs work » Needs review
FileSize
40.3 KB
3.13 KB

Hey, I have tried to cover the points mentioned in #112
Please review

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! The interdiff looks good. However, the patch itself has this in the node.creating_content.html.twig file:

+{% set content_link_text %}
+  {% trans %}Content{% endtrans %}
+{% endset %}
+{% set content_link = render_var(help_route_link(content_link_text, 'system.admin_content')) %}
+{% set content_permissions_link_text %}
+  {% trans %}Access the Content overview page{% endtrans %}
+{% endset %}
+{% set content_permissions_link = render_var(help_route_link(content_permissions_link_text, 'user.admin_permissions', {}, {'fragment': 'module-node'})) %}
+{% set content_link_text %}
+  {% trans %}Content{% endtrans %}
+{% endset %}
+{% set content_link = render_var(help_route_link(content_link_text, 'system.admin_content')) %}

content_link_text and content_link are defined twice. I guess it was already defined in patch #109. So we need to take out one set of definitions of those two variables.

shetpooja04’s picture

Status: Needs work » Needs review
FileSize
40.13 KB
998 bytes

Thanks for the clarification in #112
I have removed one instance of content_link_text and content_link which are defined twice, Please review.

jhodgdon’s picture

Thanks! This looks right to me. We still need someone to do a careful review as outlined in the issue summary under Remaining Tasks (part B - Review the patch).

daffie’s picture

A quick review with 3 strange mistakes:

  1. +++ b/core/modules/help_topics/help_topics/node.creating_content.html.twig
    @@ -0,0 +1,36 @@
    +<p>{% trans %}Create and publish a content item. See {{ content_overview_topic }}"> for more about content types and content items.{% endtrans %}</p>
    

    Why are the "> added after the link?

  2. +++ b/core/modules/help_topics/help_topics/comment.creating_type.html.twig
    @@ -0,0 +1,32 @@
    +  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Structure</em> &gt; <em>{{ comment_types_link }}</em>.{% endtrans %}</li>
    

    Why are the "> added after the link?

  3. +++ b/core/modules/help_topics/help_topics/node.editing.html.twig
    @@ -0,0 +1,39 @@
    +<p>{% trans %}Users with the <em>{{ content_permissions_link }}</em> permission can use the <em>Content</em> page to find content. Each content type has its own edit permissions. For example, to edit content of type Article, a user would need the <em>Article: Edit own content</em> permission to edit an article they created, or the <em>Article: Edit any content</em> permission to edit an article someone else created. In addition, users with the <em>Bypass content access control</em> or <em>Administer content</em> permission can edit content items of all types. Some contributed modules change the permission structure for editing content.{% endtrans %}</p>
    

    Why is > added before the link?

jhodgdon’s picture

FileSize
40.12 KB
2.72 KB

Good catch! All three of those should be removed. They were left over from before we had the link functions that we are now using to make those links -- before we had to do <a href="{{ something }}">link text</a>, and when they were converted to use the new functions, those > were missed.

I don't see the problem in the node.editing topic (#3 in your review) though... I looked at all the {{ something }} spots in the patch and they all look good to me (in my new patch).

Here's a hopefully fixed patch.

jhodgdon’s picture

Status: Needs work » Needs review
paulocs’s picture

For me we can move it to RTBC!

I didn't find any problem and I also didn't find a ">"tag before any link in node.editing.html.twig.
Cheers, Paulo.

daffie’s picture

@paulocs: Did you follow the review process from the issue summary. If so, then please change the status from "needs review" to "reviewed and tested by the community".

daffie’s picture

Status: Needs review » Needs work
FileSize
77.88 KB

I did a full review and I have 2 remarks:

  1. On the page configuring comments: We have the line: "and Creating a comment type to configure a comment type." Should configure not be replaced by create?
  2. On the page "Editing a content item" We have the link with the title: ">Access the Content overview page". Can we remove the ">" character. See:
jhodgdon’s picture

Thanks very much for taking the time to do a careful review!

Regarding item 1, that is a link to a topic whose title is "Creating a comment type" (all of the "how to do something" topics have titles starting with -ing verbs). So I think we should leave that as is.

Here's a fix for item 2.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

For 122.1: Lets leave at as it is.

I followed the review process as described in the IS.
I all looks good to me.
For me it is RTBC.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 123: 3067727-123.patch, failed testing. View results

andypost’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated failure

Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest::testFormsLinksDisabled
Failed asserting that 'unknown error: session deleted because of page crash\n
from unknown error: cannot determine loading status\n
from tab crashed\n
  (Session info: headless chrome=74.0.3729.157)\n
  (Driver info: chromedriver=2.38.552522 (437e6fbedfa8762dec75e2c5b3ddb86763dc9dcb),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)' contains "is not clickable at point".
larowlan’s picture

Status: Reviewed & tested by the community » Needs review

This reads great, kudos to all involved.

I applied this patch locally and reviewed the text of each topic from the UI, as its much easier to read them that way than from a patch file.

The only thing I've got a question about is for taxonomy. Now I realise that we have whole topics on adding fields to entity sub-types, however we do have a topic in this patch that specifically talks about how to add commenting to an entity-type. So I wonder if taxonomy is special enough to warrant a topic on on classifying content with terms, that talks about how you'd go about adding a new term-reference field.

Personally, after reading all the pages, that felt like a missing piece of the taxonomy topics. You're told how to manage terms/vocabs and how to create them, but not how to associate content with them.

Thoughts?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Good question!

The topic on adding comments to an entity type is called "Configuring comments" (comment.configuring.html.twig). It starts with a step that says "Follow the steps in the related Adding a field to an entity sub-type topic" to add the comment field. And then it has detailed instructions about the field settings, which needed to be documented somewhere what they all mean.

The similar topic for taxonomy is called "Configuring taxonomy" (taxonomy.configuring.html.twig). It starts with steps about adding a taxonomy vocabulary (which you don't have to do for comments), mentions how to optionally add a term or two while you're there, and then has a step that says "follow the instructions on the related Adding a reference field to an entity sub-type topic to add a taxonomy reference field to the entity type".

So... I think the topic you're asking for in #128 exists?

  • larowlan committed 0211e1e on 9.3.x
    Issue #3067727 by jhodgdon, batigolix, shetpooja04, NitinLama, Pooja...

  • larowlan committed 1dfa645 on 9.2.x
    Issue #3067727 by jhodgdon, batigolix, shetpooja04, NitinLama, Pooja...
larowlan’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Yes, that makes sense to me, thanks for the explanation

Committed 0211e1e and pushed to 9.3.x.

Backported to 9.2.x as help_topics is experimental

Status: Fixed » Closed (fixed)

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