Problem/Motivation
#3041924: [META] Convert hook_help() module overview text to topics for the filter, ckeditor, editor module(s).
Blocks content types issue #3067727: Convert comment, node, path, taxonomy module hook_help() to topic(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:
- 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().
- 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':
And ends either at the end of the function, or where you find another
case 'something':
line. - 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.
- 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). - 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). - 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).
- 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:
- Apply the patch.
- Turn on the experimental Help Topics module in your site, as well as the module(s) listed in this issue.
- 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. - 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].
- 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.
Comment | File | Size | Author |
---|---|---|---|
#80 | interdiff.txt | 7.24 KB | jhodgdon |
#80 | 3067614-80.patch | 12.96 KB | jhodgdon |
Comments
Comment #2
volkswagenchick@luwoldy, @eeyorr, @CelSki, and I created this issue in the first-time contributor workshop at DrupalCamp Asheville.
We reviewed the instructions from the meta ticket and had a round-robin where each new contributor created an issue so they would understand the process.
Comment #3
volkswagenchickTagging issue for DrupalCamp Colorado Contribution Day August 4, 2019 -https://2019.drupalcampcolorado.org/contribution-day
Comment #4
batigolixI'll provide a patch that can serve as a starting point.
Comment #5
jhodgdonUpdated issue summary with better instructions/guidelines
Comment #6
batigolixThis patch adds 4 help topics:
- core: formatting text
- ckeditor: what is
- ckeditor: configuring
- ckeditor: formatting text
It can serve as a starting point for this issue.
Comment #7
batigolixComment #8
batigolixComment #9
batigolixThe patch provides the help topics for the ckeditor and editor modules.
The editor module is a bit abstract for the user because it provides the framework for an actual rich text editor. I did not include a topic on how to use an editor because it would be hypothetical. I dont think that is very useful.
The top level help topic core.formatting_text is part of the help_topic module, but it could be moved to editor as editor is a dependency for any module that provides an actual rich text editor. Or should the top level topic always be available even if editor isnt enabled? Any toughts?
Please have a look at the structure and content in general.
Comment #10
jhodgdonThanks for the patch! Just a quick note that I agree with your analysis that the Editor module doesn't need a help topic, because it doesn't really provide any "tasks" for an admin to use (it is just a base module that other modules require).
Comment #11
jhodgdonThis patch needs a quick rework for these new guidelines (please read new issue summary).
Also I glanced at the patch. The HTML tags like P and H2 should go *outside* the {% trans %] tags. In this patch they seem to be inside mostly.
Comment #12
spitzialist CreditAttribution: spitzialist as a volunteer and at Unic commented- Reviewed and tested the existing patch.
- Quickly fixed the HTML tags as noticed by jhodgdon in #11.
I did not yet work on including the new guidelines, so still needs work.
Comment #13
batigolixComment #14
jhodgdonWe've migrated the help topic standards to https://www.drupal.org/docs/develop/documenting-your-project/help-topic-... so updating issue summary again.
Comment #15
anmolgoyal74 CreditAttribution: anmolgoyal74 commentedComment #16
jhodgdonThanks for the new patch!
I didn't apply the patch and view it inside Drupal (so I haven't tested it -- that still needs to be done). But I did read through the patch text carefully. Some comments:
a) In several of the topics, there are lines saying "For more information...". Please check the https://www.drupal.org/docs/develop/documenting-your-project/help-topic-... -- we have a section called "Additional resources" at the end of topics, which would be a better place to put those links.
b) You don't need to use Twig variables for static URLs like:
You can just put those URLs into the template directly. You only need to use Twig variables for Drupal route URLs, like
c) Under Goal, let's not use an -ing verb. For example, you have:
Let's make that something like... Hm. that text doesn't even make sense to me... how about:
d) Well I kind of gave up here. Please go read the help topic standards. It's a good start, because the first topic I looked at (Formatting text with CKEditor) starts with Goal, but the Goal should be just a short sentence or at most maybe 2 sentences, not several paragraphs. And then you can have maybe a "What is ..." heading, and then "Steps". But that is not the structure of this topic.
So... Please restructure the topics so that they follow the help topic standards. Thanks!
Comment #17
volkswagenchickTagging for badcamp2019, thanks! (October 2-5)
Comment #19
jhodgdonWe 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.
Comment #20
nishantghetiya CreditAttribution: nishantghetiya at QED42 commentedComment #21
jhodgdon@nishantghetiya -- are you still planning to work on this? If not, please unassign so someone else can. Thanks!
Comment #22
hash6 CreditAttribution: hash6 at QED42 commented@nishantghetiya I have picked this up.
Comment #23
hash6 CreditAttribution: hash6 at QED42 commented- only one task topic retained for ckeditor: ckeditor.configure and it includes goal and steps.
- moved content from ckeditor.formatting_text to ckeditor.overview.html.twig
- Moved content from ckeditor.what_is to ckeditor.overview.html.twig
- Moved editor.configuring to editor.overview
@jhodgdon Please have a look at it, let me know your inputs.
Thanks !!
Comment #24
hash6 CreditAttribution: hash6 at QED42 commentedComment #25
hash6 CreditAttribution: hash6 at QED42 commentedComment #28
jhodgdonLooks like the patch in #23 does not apply. I will wait for a green test result (all passing) before I review. Thanks!
Comment #29
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have re-rolled patch #23
Comment #31
abhisekmazumdarComment #32
abhisekmazumdarComment #34
hash6 CreditAttribution: hash6 at QED42 commentedComment #35
Scott Weston CreditAttribution: Scott Weston at Bounteous commentedComment #36
Scott Weston CreditAttribution: Scott Weston at Bounteous commentedComment #37
Scott Weston CreditAttribution: Scott Weston at Bounteous commentedComment #38
jhodgdonThanks for the patches!
So, first off, please read the issue summary here and the Standards for Help Topics if you haven't done so already.
What we're looking for here is ***topic-based help*** , which is not the same as module overviews.
So for instance, in the overview topic, there's a heading called "What is CKEditor?" and it starts off with "The CKEditor module...". Meaning that this section is an overview of the CKEditor module. That is not what we're looking for.
What we want instead is to think about what tasks people would need to do related to these modules, and write topics for each task. If a task requires some background information, then we put in headers like "What is a..." to give the background information. And at some point, we do want to tell people (probably at the end of a paragraph somewhere) that a particular Drupal Core module (and we do need to be specific and say it is a Drupal Core module by the way!) provides some functionality.
The help topic standards are also very specific about what the h2-level headings should be in task-oriented topics, as well as section topics. Many of the headings in the Overview topic in this patch do not conform to the standards.
So... this patch needs some rewriting... thanks!
Comment #39
Scott Weston CreditAttribution: Scott Weston at Bounteous commented@jhodgdon thank you for the feedback. I will write the help text per the gudelines today and submit for review.
(Edit: I answered my own question by reading)
Comment #40
Scott Weston CreditAttribution: Scott Weston at Bounteous commentedComment #41
jhodgdonYou were asking about text formats in a comment that you edited... Text formats seem like a good thing to cover in the core namespace, rather than in the editor/ckeditor namespace (i..e., a topic with file name core.something.html.twig). Could be part of this patch -- maybe in the Overview topic, which could then be called something like "Configuring editing and text formats"?
Comment #42
jhodgdonActually... Text formats are provided by the Filter module, which is currently part of #3067727: Convert comment, node, path, taxonomy module hook_help() to topic(s).
It seems like writing help for editors kind of depends on having help available about text formats. So... I think we have two choices:
a) Postpone this issue until the one about the Filter module is done (it is currently postponed until the field modules help is done #3087562: Convert datetime, datetime_range, field, field_ui, link, options, telephone, text modules hook_help() to topic(s)
b) Move the filter module from that other issue to be part of this issue instead.
I would prefer (b), so I have tentatively done that (we can always change it back).
Comment #43
Scott Weston CreditAttribution: Scott Weston at Bounteous commentedI created help topics for Editor, Filter, and CK Editor following the help topic standards (hopefully). Please review and provide feedback on any changes to content, voice, etc. Also, if additional related topics are desired for this issue, I can add them as requested.
Comment #44
Scott Weston CreditAttribution: Scott Weston at Bounteous commentedComment #45
Scott Weston CreditAttribution: Scott Weston at Bounteous commentedComment #46
jhodgdonThis is blocking progress on other issues, so adding tag.
Sorry this hasn't been reviewed yet. We'll get to it eventually. The two people who have been reviewing these issues have both been mostly unavailable for several weeks... also the ones that are reviewed/tested (RTBC) are not getting committed, which has stalled progress.
Comment #47
benjifisherI am reviewing this issue at MidCamp 2020, so I am assigning it to myself and adding the issue tag.
Comment #48
benjifisherAre we avoiding the term "WYSIWYG"? The phrase "visual text editor" seems sort of academic.
Should we mention in the introductory paragraph of the CKEditor overview that we are using the external CKEditor library (with a link in the Additional resources section)? It might help avoid confusion. Maybe also add "library" to that link text.
This is the first of several lines where there is a missing space after "endtrans". I see the same thing in at least one of the other files, too.
The page title seems to be "Text formats and editors".
Remove "button" after + Add text format.
I would put the "Complete or update …" step at the end of the list. Probably reword it, something like "Make any other changes to the configuration form and click Save configuration at the bottom of the page. See also my comments on
filter.overview.html.twig
below.Delete the definite article: "When dragging a button, Active toolbar shows …".
Delete "button": it should be "such as Image,".
The indentation is off, and there are stray spaces around "Online documentation for the CKEditor module" and "CKEditor website" in the link text. The actual title for the first link is "CKEditor module overview". As I said above, consider adding "library" to the second item’s link text. The title on that page is not really helpful.
Why "menu" (twice) instead of "page"? (This also shows "endtrans%}" without a space.)
Remove the stray space before the period.
There are stray spaces around the link text. While you are at it, change the link text to something like "Overview page for the Text Editor module".
I see the same problems here as in the first two files:
endtrans%}
missing a space, stray spaces around the link text, and we should change that link text anyway.The
content_url
Twig variable is never defined, so this ends up being an emptyhref
.This page contains generic instructions for Text Editor add-on modules, and most sites will only have the core CKEditor installed. I think the summary paragraph should suggest going to the more specific page unless the site builder has installed some non-core editor module. Alternatively, remove
filter.overview.html.twig
and move some of its content to this page.I see one of the same problems here as in the first two files:
endtrans%}
missing a space.Should it be "on the site" instead of "in the site"?
Suggestion: end the first sentence with a colon, remove "These are", and make the three items into an unformatted list. I suppose this suggestion has the disadvantage that we either split up the translatable text or else include HTML tags in it. Maybe "the type of a field" instead of "the type of field".
Same as in the first file: the page title is "Text formats and editors".
Remove "button".
Consider putting the items after this one in a nested list, and then add a "click save" step at the end (not nested).
I do not like "human name". Maybe "human-friendly name" or "human-readable name" or either one with a space instead of a hyphen.
This should be something like "If any enabled filters have configurable settings, then these will appear …". Maybe leave out "then" for stylistic consistency.
Maybe
ckeditor.overview.html.twig
should add this as a related page. Some steps are duplicated, and others are described only on this page. I see that both of these pages are related toeditor.overview.html.twig
, but that means that it takes two steps to get here from there.Comment #49
jhodgdonThanks for the review! I'll make a new patch and attempt to fix these issues.
Comment #50
jhodgdonIn addition to the review above, I think we should actually reconsider the list of topics that are in this patch... It has 4 topics in it:
1) Managing CKEditor -- top-level
2) Installing text editors -- related to managing editors and managing ckeditor topics
3) Managing text editors -- top-level
4) Managing text filters and text formats -- top-level
These still seem kind of linked to modules rather than task-oriented... What I actually think we might want instead is:
a) Managing text filters and text formats
b) Connecting editors to text formats
c) Confiuring CKEditor
These three can be linked together, as well as linked to the existing topics on configuring content structure and setting up the content editing form. Also, I don't know if they should be top level. Does that make sense?
Comment #51
benjifisherI definitely agree that we want at most one top-level (overview) topic based on the current patch.
Should we have no top-level topics, but get here from existing topics?
Arguments in favor of linking from the existing "Managing content structure" topic:
Arguments against:
I guess I lean towards adding a new, top-level topic for filters/formats/editors. Maybe that is just because I am used to it. On the other hand, I would rather err on the side of too many top-level topics, expecting that we will have some reorganization as we get closer to a stable version of the module.
So my suggestion is to have the three task topics that you suggest plus one overview topic:
Maybe you have better ideas than I do for naming the files. This organization would get the security information one click away from the main Help page. It does mean a lot of clicks to get to the CKEditor page, but maybe that is OK.
Comment #52
jhodgdonRegarding "Filters are important for site security." -- the solution to that argument against is to make sure that the filters topic is marked as "related" to the "Making your site secure" top-level topic, which we already have (core.security.html.twig).
So, I'm inclined not to have a top-level topic about filters, and instead put the background information into the filter.format/filter.managing task topic that you proposed, and add core.security to the Related section for that topic.
Thoughts?
Comment #53
benjifisher@jhodgdon, that seems like a good approach.
Maybe we should also have a follow-up issue to rewrite the "Managing content structure" topic, if you agree with my complaint that the current text is too abstract.
Comment #54
jhodgdonI think that we actually need that text about entity type and sub-type in the help, because we need to define those terms in order to be able to say that you can attach fields to all of those things. We could try to find another term, but I think it's best to just use the standard terms that people might find in other Drupal docs. We did something similar in the User Guide, after much discussion. That topic is meant to provide background information... But if you think something is unclear or not useful, please do file an issue.
Anyway, meanwhile it seems like we have a path forward for this issue.
Comment #55
jhodgdonI'm working on a new patch for this issue today.
Comment #56
jhodgdonHere is a new patch. Too much changed, so I didn't make an interdiff file.
I decided that all of the information could fit well into two topics. Neither is top level. You can reach both of them from the "Configuring content structure" topic, and the filter topic is also reachable from the Security topic.
I think I also addressed all of the very helpful review comments in #48. Note that I removed all of the "additional resources" links, as I didn't think they really had any useful information in them. See what you think...
Comment #57
jhodgdonComment #58
benjifisherThanks, this is a lot cleaner than the first version I reviewed. That was too early for me to go into copy-editor mode, but now I think that mode is appropriate.
I am late in making this suggestion, since #3087562: Convert datetime, datetime_range, field, field_ui, link, options, telephone, text modules hook_help() to topic(s) is already fixed, but I would like add a page on text fields, similar to
field_ui.reference_field
, and link this page tofilter.overview
. I would further like to link thefield_ui
and newtext
pages tofield_ui.add_field
and remove the links fromfield_ui.reference_field
tofield_ui.manage_form
andcore.content_structure
. This would provide another way to navigate tofilter.overview
: fromcore.content_structure
orfield_ui.manage_form
tofield_ui.add_field
to the newtext
page tofilter.overview
. If you think this is worth considering, then I will add a follow-up issue.In
filter.overview.html.twig
:There should be commas around the appositive phrase "using the core Text Editor module", but I think it would be even better to split up this long sentence:
Do we need a hyphen here?
Under Steps:
I do not think "Note that" is useful here.
You can only edit the machine name when adding a new format, not once it has been saved. Maybe we should not even mention machine names here: less is more.
I think you are right. I am trying to think why I might need to be directed to the page for the CKEditor library. Maybe I am looking for additional CKEditor plugins. This is certainly a reasonable scenario, but I think it is out of scope for help topics from Drupal core.
Comment #59
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commentedComment #60
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commentedKindly review new patch with suggestions of @benjifisher
Comment #61
abhisekmazumdarHi @prabha1997 the .patch file is empty kindly check.
Comment #62
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commented@abhisekmazumdar Thanks for review.Here i upload new patch kindly review
Comment #63
jhodgdonThis patch now has only 1 of the 3 topic files in it. Also it does not apply. Please make a new patch.
Also, note that #57 suggested that we need a new topic added, so please put that in your patch. Thanks!
Comment #64
neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commentedComment #65
neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commentedHello @jhodgdon
I have applied points as asked in #58, except 1st point.
Can you please make me clear with it more.
Kindly review patch.
Thanks
Comment #66
neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commentedComment #67
neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commentedComment #68
jhodgdonRegarding the first point of #58, the reviewer is asking for a new topic to be added.
Also, the fix for point 5 in #58 is not right. Please take another look. The sentence that is in the patch does not make sense. Here is the change:
You need to take out "provided by modules and installation profiles (only for formats added by administrators)" because that only applies to machine names, which we have decided not to talk about in this topic.
Comment #69
neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commentedUpdated the changes for 5th point.
Kindly review the patch.
Comment #70
jhodgdonThanks, that looks better (based on the interdiff)! So all we need for the review in #58 is the new topic (point 1). I see now that the suggestion was to do it in a follow-up issue... but I think we should just do it here if we're going to do it and it is needed to properly document filters?
Comment #71
jhodgdonOh, so setting back to Needs Work based on that... but I think the rest of the patch is good. Thanks everyone who has contributed so far!
Comment #72
jhodgdonHere is a new patch. I thought about making a new topic similar to field_ui.reference_field (as suggested in #58), but when I looked at that topic, it didn't make sense to me. The topic title is "Adding a reference field to an entity sub-type", and the reason we have that topic is that there are settings on the reference fields that can be confusing. But I don't think the same applies to text fields -- once you choose the right field type, it is pretty straightforward to add the field.
So, what I did instead was to add a couple of sections to core.content_structure, to explain the types of text fields. Since I was doing that, I thought that explaining the List fields also made sense... which isn't totally related to this patch, but I put it in anyway... hope that is OK.
And then I added some additional cross-links to the filter topic, so that it is visible from more topics.
See what you think?
Comment #74
andypostUpdated summary with link to #3067727: Convert comment, node, path, taxonomy module hook_help() to topic(s) which is blocked on the subject
reminds me #1380720: Rename entity "bundle" to "subtype" in the UI text and help
Just 5c on relations
I think that filters are more related to text module and core's "string" field, so field_ui.* should not be mentioned here
Additionally this topic is blocker for all content entities which have body/description formatted...
maybe split it out to own issue but editor part also more relatated to text i/o
Comment #75
andypostComment #76
jhodgdonPersonally, I think that having several links under Related Topics is not a bad thing. Text formats are related to setting up text fields, and they have impact on how fields are displayed. So I think it is relevant to connect them to the field_ui* topics. For example, someone who was adding a field might see settings related to text formats, so having that documentation linked at the bottom of the page could be helpful.
Comment #77
pratik_kambleComment #78
pratik_kambleI agree with @jhodgdon for having the
I have checked the patch in #72. I have verified the patch using the tasks mentioned for the review process against branch 9.1.
The text is written in clear, simple, and in straightforward language. Validated HTML, no issues found.
Links within the text redirects to appropriate pages.
Adheres to Standards for Help Topics.
Verified the grammar for the patch and found few minor issues for punctuation, grammar, and rephrased one sentence. I feel few changes were needed so attaching a new patch with correction.
Comment #79
pratik_kambleComment #80
jhodgdonThanks for the careful review and the patch! I like some of the changes you suggested, but not all of them. The ones that need some attention:
a)
The change here takes
There are different list field types depending on the type of the value code
and makes it into
Different list field types depend on the type of the value code
I think this is confusing... I have made another suggestion in this new patch. See what you think...
b)
This change added "the" in "buttons that have the configuration". That is incorrect and should be removed.
c)
If you look at the style of the dt/dt elements in this list, the dt uses a verb in form like "Convert" (that is UI text), and the dt uses a verb in form like "Takes" or "Restricts" (to explain what it does). So the change of restricts to restrict in this line is inconsistent with the rest of the DL list.
d)
Removing this comma makes the sentence very long without a break. Maybe we should make it into two sentences instead? I actually reordered it in this new patch.
Anyway, here's a new patch with my suggested changes to your suggested changes. Take a look!
Comment #81
pratik_kambleComment #82
pratik_kambleThanks @jhodgdon, for pointing out the issue. I agreed with your suggestion. I have reviewed the patch.
Validated both grammar and HTML. Language looks simple and straight forward.
Comment #83
larowlanAre these changes expected here if this is about text formats and editors?
I tried this out locally, and it wasn't immediately obvious how to find these topics from the top-level listing. e.g. I wouldn't have immediately thought to look in 'managing content structure'. Out of scope here, but is there a plan for a top-level 'Configuration tasks' or similar topic to group configuration related duties?
Comment #84
andypost@larowlan Yes, it's #2687107: Reorganize topics into sensible outline, and/or write more topics
Comment #86
larowlanRe #83 I see this above from @jhodgdon
Technically its probably out of scope, but the text was being revised here to add extra links to text formats/editors added here so its in the same vein.
In addition the number of patches being worked on for help-topics is such that those changes are unlikely to cause conflicts with other patches and this module is experimental.
Further to this, this issue has been flagged as blocking other help-topics patches so in the interest of progress I'm going to be pragmatic about it.
I tried this patch out locally and read the text in the UI. It does a great job of explaining a powerful feature of Drupal and makes sure to provide emphasis to the security related aspects of this configuration task.
Committed c6012dc and pushed to 9.1.x. Thanks!