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:

  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':
    

    And ends either at the end of the function, or where you find another 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
#80 interdiff.txt7.24 KBjhodgdon
#80 3067614-80.patch12.96 KBjhodgdon
#78 interdiff.txt11.21 KBpratik_kamble
#78 3067614-78.patch13 KBpratik_kamble
#72 interdiff-69-72.txt5.43 KBjhodgdon
#72 3067614-72.patch13.02 KBjhodgdon
#69 3067614-69.patch8.1 KBneelam_wadhwani
#69 interdiff_65-69.txt1.69 KBneelam_wadhwani
#65 3067614-65.patch8.19 KBneelam_wadhwani
#65 interdiff_56-65.txt4.18 KBneelam_wadhwani
#62 interdiff_56-62.txt11.89 KBprabha1997
#62 3067614-62.patch4.15 KBprabha1997
#60 interdiff_56-60.txt7.89 KBprabha1997
#60 3067614-60.patch0 bytesprabha1997
#56 3067614-filter-help-56.patch8.25 KBjhodgdon
#43 interdiff_36-42.txt16.79 KBscott weston
#43 help-topics-filter-editor-ckeditor-3067614-43.patch10.82 KBscott weston
#36 interdiff_35-36.txt650 bytesscott weston
#36 help-topic-ckeditor-editor-3067614-36.patch8.7 KBscott weston
#35 interdiff_32-35.txt1.21 KBscott weston
#35 help-topic-ckeditor-editor-3067614-35.patch8.7 KBscott weston
#32 interdiff.3067614.29-32.txt9.07 KBabhisekmazumdar
#32 help-topic-ckeditor-editor-3067614-32.patch8.7 KBabhisekmazumdar
#29 3067614-29.patch8.65 KBravi.shankar
#23 interdiff_15-23.txt15.13 KBhash6
#23 help-topic-ckeditor-editor-30676141-23.patch8.78 KBhash6
#15 interdiff_12_15.txt6.56 KBanmolgoyal74
#15 help-topic-ckeditor-editor-30676141-15.patch7.51 KBanmolgoyal74
#12 help-topic-ckeditor-editor-30676141-12.patch7.92 KBspitzialist
#9 help-topic-ckeditor-editor-30676141-9.patch7.96 KBbatigolix
#6 help-topic-ckeditor-editor-30676141-6.patch5.72 KBbatigolix

Comments

luwoldy created an issue. See original summary.

volkswagenchick’s picture

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

volkswagenchick’s picture

Issue tags: +dcco2019

Tagging issue for DrupalCamp Colorado Contribution Day August 4, 2019 -https://2019.drupalcampcolorado.org/contribution-day

batigolix’s picture

Assigned: Unassigned » batigolix

I'll provide a patch that can serve as a starting point.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary with better instructions/guidelines

batigolix’s picture

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

batigolix’s picture

Assigned: batigolix » Unassigned
batigolix’s picture

Assigned: Unassigned » batigolix
batigolix’s picture

Status: Active » Needs review
StatusFileSize
new7.96 KB

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

jhodgdon’s picture

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

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Needs work

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

spitzialist’s picture

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

batigolix’s picture

Assigned: batigolix » Unassigned
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.

anmolgoyal74’s picture

Status: Needs work » Needs review
StatusFileSize
new7.51 KB
new6.56 KB
jhodgdon’s picture

Status: Needs review » Needs work

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

+{% set user_guide = render_var('https://www.drupal.org/docs/user_guide/en/structure-text-format-config.html') %}
+{% set ckeditor_ally = render_var('http://docs.ckeditor.com/#!/guide/dev_a11y') %}
+{% set ckeditor_shortcuts = render_var('http://docs.ckeditor.com/#!/guide/dev_shortcuts') %}

You can just put those URLs into the template directly. You only need to use Twig variables for Drupal route URLs, like

+{% set formats = render_var(url('filter.admin_overview')) %}

c) Under Goal, let's not use an -ing verb. For example, you have:

Formatting content for individual text formats

Let's make that something like... Hm. that text doesn't even make sense to me... how about:

Configure CKEditor for a text format.

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!

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.

nishantghetiya’s picture

Assigned: Unassigned » nishantghetiya
jhodgdon’s picture

@nishantghetiya -- are you still planning to work on this? If not, please unassign so someone else can. Thanks!

hash6’s picture

Assigned: nishantghetiya » hash6

@nishantghetiya I have picked this up.

hash6’s picture

- 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 !!

hash6’s picture

Status: Needs work » Needs review
hash6’s picture

Assigned: hash6 » Unassigned

The last submitted patch, 12: help-topic-ckeditor-editor-30676141-12.patch, failed testing. View results

The last submitted patch, 6: help-topic-ckeditor-editor-30676141-6.patch, failed testing. View results

jhodgdon’s picture

Status: Needs review » Needs work

Looks like the patch in #23 does not apply. I will wait for a green test result (all passing) before I review. Thanks!

ravi.shankar’s picture

Status: Needs work » Needs review
StatusFileSize
new8.65 KB

I have re-rolled patch #23

Status: Needs review » Needs work

The last submitted patch, 29: 3067614-29.patch, failed testing. View results

abhisekmazumdar’s picture

Assigned: Unassigned » abhisekmazumdar
abhisekmazumdar’s picture

Assigned: abhisekmazumdar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new8.7 KB
new9.07 KB

Status: Needs review » Needs work

The last submitted patch, 32: help-topic-ckeditor-editor-3067614-32.patch, failed testing. View results

hash6’s picture

Assigned: Unassigned » hash6
scott weston’s picture

scott weston’s picture

scott weston’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

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

scott weston’s picture

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

scott weston’s picture

Assigned: hash6 » scott weston
jhodgdon’s picture

You 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"?

jhodgdon’s picture

Title: Convert ckeditor, editor module hook_help() to topic(s) » Convert filter, ckeditor, editor module hook_help() to topic(s)
Issue summary: View changes

Actually... 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).

scott weston’s picture

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

scott weston’s picture

Status: Needs work » Needs review
scott weston’s picture

Assigned: scott weston » Unassigned
jhodgdon’s picture

Issue tags: +blocker

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

benjifisher’s picture

Assigned: Unassigned » benjifisher
Issue tags: +midcamp2020

I am reviewing this issue at MidCamp 2020, so I am assigning it to myself and adding the issue tag.

benjifisher’s picture

Assigned: benjifisher » Unassigned
Status: Needs review » Needs work
  1. Are we avoiding the term "WYSIWYG"? The phrase "visual text editor" seems sort of academic.

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

  3.   +++ b/core/modules/help_topics/help_topics/ckeditor.overview.html.twig
      @@ -0,0 +1,27 @@
      ...
      +<h2>{% trans %}Goal{% endtrans%}</h2>

    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.

  4.   +  <li>{% trans %}In the <em>Configuration</em> administrative menu, navigate to <em>Content Authoring</em> > <a href="{{ content_url }}"><em>Text editors and formats</em></a>. The <em>Text Editor</em> column on the page displays the configured text editor for each text format.{% endtrans%}</li>

    The page title seems to be "Text formats and editors".

  5.   +  <li>{% trans %}Click <em>Configure</em> next to the text format to configure, or click <em>+ Add text format</em> button to create a new text format.{% endtrans%}</li>

    Remove "button" after + Add text format.

  6.   +  <li>{% trans %}Complete or update the options on the configuration form.{% endtrans%}</li>
      +  <li>{% trans %}The <em>Name</em> field is the human name for this text format.{% endtrans%}</li>

    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.

  7.   +  <li>{% trans %}To make an available button active, drag the button from <em>Available buttons</em> and place it into <em>Active toolbar</em> in the desired location. When dragging a button, the <em>Active toolbar</em> shows available button groups.{% endtrans%}</li>

    Delete the definite article: "When dragging a button, Active toolbar shows …".

  8.   +  <li>{% trans %}Any active buttons that have configurable settings, such as <em>Image</em> button, are configurable in the <em>CKEditor plugin settings</em> section that appears below the <em>Toolbar configuration</em> section.{% endtrans%}</li>

    Delete "button": it should be "such as Image,".

  9.   +<ul>
      +    <li>{% trans %}<a href="https://www.drupal.org/documentation/modules/ckeditor"> Online documentation for the CKEditor module </a>{% endtrans %}</li>
      +    <li>{% trans %}<a href="http://ckeditor.com/"> CKEditor website </a>{% endtrans %}</li>
      +</ul>

    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.

  10.   +++ b/core/modules/help_topics/help_topics/editor.install.html.twig
      @@ -0,0 +1,21 @@
      ...
      +<p>{% trans %}Drupal core provides CKEditor as a module (which can be enabled in the <a href="{{ extend }}"><em>Extend</em></a> administrative menu). The Text Editor module provides a framework for other modules to provide alternative text editors.{% endtrans%}</p>
      +<h2>{% trans %}Steps{% endtrans%}</h2>
      +<ol>
      +  <li>{% trans %}Enable the desired text editor module on the <a href="{{ extend }}"><em>Extend</em></a> administrative menu. This can be either the core CKEditor module, or an editor provided by a Drupal contributed module. If enabling the core CKEditor, proceed to <em>Managing CKEditor</em> in related topics below. The following steps are for editors other than CKEditor.{% endtrans%}</li>

    Why "menu" (twice) instead of "page"? (This also shows "endtrans%}" without a space.)

  11.   +  <li>{% trans %}After the text editor is installed, it may provide configuration options specific to the editor . Refer to the contributed module's documentation for information on configuration options.{% endtrans%}</li>

    Remove the stray space before the period.

  12.   +  <li>{% trans %}<a href="https://www.drupal.org/documentation/modules/editor"> Online documentation for the Text Editor module </a>{% endtrans %}</li>

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

  13.   +++ b/core/modules/help_topics/help_topics/editor.overview.html.twig
      @@ -0,0 +1,24 @@

    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.

  14.   +  <li>{% trans %}In the <em>Configuration</em> administrative menu, navigate to <em>Content Authoring</em> > <a href="{{ content_url }}"><em>Text editors and formats</em></a>. The <em>Text Editor</em> column on the page displays the configured text editor for each text format.{% endtrans%}</li>

    The content_url Twig variable is never defined, so this ends up being an empty href.

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

  16.   +++ b/core/modules/help_topics/help_topics/filter.overview.html.twig
      @@ -0,0 +1,25 @@

    I see one of the same problems here as in the first two files: endtrans%} missing a space.

  17.   +<p>{% trans %}The Filter module allows administrators to configure text formats. Text formats change how HTML tags and other text will be <em>processed and displayed</em> in the site. They are used to transform text, and also help to defend your web site against potentially damaging input from malicious users. Visual text editors can be associated with text formats by using the Text Editor module.</p>{% endtrans %}

    Should it be "on the site" instead of "in the site"?

  18.   +<p>{% trans %}Text fields that allow text formats are those with "formatted" in the description. These are <em>Text (formatted, long, with summary)</em>, <em>Text (formatted)</em>, and <em>Text (formatted, long)</em>. You cannot change the type of field once a field has been created.{% endtrans%}</p>

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

  19.   +  <li>{% trans %}In the <em>Configuration</em> administrative menu, navigate to <em>Content Authoring</em> > <a href="{{ content_url }}"><em>Text editors and formats</em></a>.{% endtrans%}</li>

    Same as in the first file: the page title is "Text formats and editors".

  20.   +  <li>{% trans %}Click <em>Configure</em> next to the text format to configure, or click <em>+ Add text format</em> button to create a new text format.{% endtrans%}</li>

    Remove "button".

  21.   +  <li>{% trans %}Complete or update the options on the configuration form.{% endtrans%}</li>

    Consider putting the items after this one in a nested list, and then add a "click save" step at the end (not nested).

  22.   +  <li>{% trans %}The <em>Name</em> field is the human name for this text format.{% endtrans%}</li>

    I do not like "human name". Maybe "human-friendly name" or "human-readable name" or either one with a space instead of a hyphen.

  23.   +  <li>{% trans %}If any enabled filters with configurable settings will appear in the <em>Filter settings</em> section.{% endtrans%}</li>

    This should be something like "If any enabled filters have configurable settings, then these will appear …". Maybe leave out "then" for stylistic consistency.

  24. 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 to editor.overview.html.twig, but that means that it takes two steps to get here from there.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Thanks for the review! I'll make a new patch and attempt to fix these issues.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

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

benjifisher’s picture

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

  • This topic is broad enough that it includes all fieldable entities (not just node = "content") so it makes some sense to start there.
  • Many sites will be fine with the default configuration.

Arguments against:

  • Filters are important for site security.
  • They are considered important enough that they get their own section "Configuration > Content authoring" in the admin menu (with no other items provided by core/standard profile).
  • The existing text on that topic is pretty abstract, with phrases like "entity sub-type".

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:

  • filter.overview: Make this a top-level topic; just an overview, no tasks/steps; include some material from filter.overview and editor.overview in the current patch.
  • filter.managing or filter.format: link to the overview page; same as "Managing text filters and text formats" from #50; include steps from filter.overview in the current patch.
  • editor.managing or editor.format: link to the previous topic; same as "Connecting editors to text formats" from #50; include steps from editor.overview in the current patch
  • ckeditor.???: link to the previous topic; same as "Confi[g]uring CKEditor" from #50; include steps from ckeditor.overview in the current patch, but removing duplicate steps.

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.

jhodgdon’s picture

Regarding "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?

benjifisher’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

I'm working on a new patch for this issue today.

jhodgdon’s picture

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

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
benjifisher’s picture

Status: Needs review » Needs work

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

  1. 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 to filter.overview. I would further like to link the field_ui and new text pages to field_ui.add_field and remove the links from field_ui.reference_field to field_ui.manage_form and core.content_structure. This would provide another way to navigate to filter.overview: from core.content_structure or field_ui.manage_form to field_ui.add_field to the new text page to filter.overview. If you think this is worth considering, then I will add a follow-up issue.

  2. In filter.overview.html.twig:

    Administrators can configure text formats, assign permissions for who can use each format, and using the core Text Editor module, associate visual editors with text formats.

    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:

    Administrators can configure text formats and assign permissions for who can use each format. If the core Text Editor module is enabled, administrators can also associate visual editors with text formats.

  3. Some of the more commonly-used text filters are:

    Do we need a hyphen here?

  4. Under Steps:

    Note that if you do not have the core Text Editor module installed …

    I do not think "Note that" is useful here.

  5. Enter the desired Name for the text format, and optionally the machine name; you cannot edit the machine name for text formats provided by modules and installation profiles (only for formats added by administrators).

    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.

  6. Note that I removed all of the "additional resources" links, as I didn’t think they really had any useful information in them.

    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.

prabha1997’s picture

Assigned: Unassigned » prabha1997
prabha1997’s picture

Assigned: prabha1997 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new0 bytes
new7.89 KB

Kindly review new patch with suggestions of @benjifisher

abhisekmazumdar’s picture

Status: Needs review » Needs work

Hi @prabha1997 the .patch file is empty kindly check.

prabha1997’s picture

Status: Needs work » Needs review
StatusFileSize
new4.15 KB
new11.89 KB

@abhisekmazumdar Thanks for review.Here i upload new patch kindly review

jhodgdon’s picture

Status: Needs review » Needs work

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

neelam_wadhwani’s picture

Assigned: Unassigned » neelam_wadhwani
neelam_wadhwani’s picture

StatusFileSize
new4.18 KB
new8.19 KB

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

neelam_wadhwani’s picture

Status: Needs work » Needs review
neelam_wadhwani’s picture

Assigned: neelam_wadhwani » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

Regarding 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:

-  <li>{% trans %}Enter the desired <em>Name</em> for the text format, and optionally the machine name; you cannot edit the machine name for text formats provided by modules and installation profiles (only for formats added by administrators).{% endtrans %}</li>
+  <li>{% trans %}Enter the desired <em>Name</em> for the text format provided by modules and installation profiles (only for formats added by administrators).{% endtrans %}</li>

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.

neelam_wadhwani’s picture

Status: Needs work » Needs review
StatusFileSize
new1.69 KB
new8.1 KB

Updated the changes for 5th point.
Kindly review the patch.

jhodgdon’s picture

Thanks, 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?

jhodgdon’s picture

Status: Needs review » Needs work

Oh, 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!

jhodgdon’s picture

Status: Needs work » Needs review
StatusFileSize
new13.02 KB
new5.43 KB

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

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

Issue summary: View changes

Updated summary with link to #3067727: Convert comment, node, path, taxonomy module hook_help() to topic(s) which is blocked on the subject

field to an entity sub-type

reminds me #1380720: Rename entity "bundle" to "subtype" in the UI text and help

Just 5c on relations

+++ b/core/modules/help_topics/help_topics/editor.overview.html.twig
@@ -0,0 +1,23 @@
+label: 'Connecting text editors to text formats'
...
+  - field_ui.manage_form
+  - filter.overview

+++ b/core/modules/help_topics/help_topics/filter.overview.html.twig
@@ -0,0 +1,40 @@
+label: 'Managing text filters and text formats'
+related:
+  - core.security
+  - core.content_structure
+  - field_ui.manage_form
+  - field_ui.manage_display
+  - field_ui.add_field
...
+{% set content_url = render_var(url('filter.admin_overview')) %}

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

andypost’s picture

jhodgdon’s picture

Personally, 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.

pratik_kamble’s picture

pratik_kamble’s picture

StatusFileSize
new13 KB
new11.21 KB

I agree with @jhodgdon for having the

Text formats are related to setting up text fields, and they have an impact on how fields are displayed.

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.

pratik_kamble’s picture

jhodgdon’s picture

StatusFileSize
new12.96 KB
new7.24 KB

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

-<p>{% trans %}List fields associate pre-defined <em>keys</em> (or value codes) with <em>labels</em> that the user sees. For example, you might define a list field that shows the user the names of several locations, while behind the scenes a location code is stored in the database. There are different list field types depending on the type of the value code; for example, a <em>List (integer)</em> field has integer values, while the values of a <em>List (text)</em> field are text strings. Once you have chosen the field type, the main setting for a list field is the <em>Allowed values</em> list, which associates the keys with the labels.{% endtrans %}</p>
+<p>{% trans %}List fields associate pre-defined <em>keys</em> (or value codes) with <em>labels</em> that the user sees. For example, you might define a list field that shows the user the names of several locations, while behind the scenes a location code is stored in the database. Different list field types depend on the type of the value code; for example, a <em>List (integer)</em> field has integer values, while the values of a <em>List (text)</em> field are text strings. Once you have chosen the field type, the main setting for a list field is the <em>Allowed values</em> list, which associates the keys with the labels.{% endtrans %}</p>

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)

-  <li>{% trans %}Under <em>CKEditor plugin settings</em>, configure any buttons that have configuration. This section will not be present if there are no active buttons with configuration.{% endtrans %}</li>
+  <li>{% trans %}Under <em>CKEditor plugin settings</em>, configure any buttons that have the configuration. This section will not be present if there are no active buttons with configuration.{% endtrans %}</li>

This change added "the" in "buttons that have the configuration". That is incorrect and should be removed.

c)

   <dt>{% trans %}Convert URLs into links{% endtrans %}</dt>
   <dd>{% trans %}Takes plain URLs in text and turns them into HTML links.{% endtrans %}</dd>
   <dt>{% trans %}Restrict images to this site{% endtrans %}</dt>
-  <dd>{% trans %}For text formats that allow HTML image tags, restricts images to URLs within this site.{% endtrans %}</dd>
+  <dd>{% trans %}For text formats that allow HTML image tags, restrict images to URLs within this site.{% endtrans %}</dd>

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)

-  <li>{% trans %}Check the <em>Roles</em> that can use this text format. To ensure security, anonymous and untrusted users should only have access to text formats that restrict them to either plain text or a safe set of HTML tags, because HTML tags can allow embedding malicious links or scripts in text. <strong>Improper text format configuration is a security risk.</strong>{% endtrans %}</li>
+  <li>{% trans %}Check the <em>Roles</em> that can use this text format. To ensure security, anonymous and untrusted users should only have access to text formats that restrict them to either plain text or a safe set of HTML tags because HTML tags can allow embedding malicious links or scripts in text. <strong>Improper text format configuration is a security risk.</strong>{% endtrans %}</li>

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!

pratik_kamble’s picture

Assigned: Unassigned » pratik_kamble
pratik_kamble’s picture

Assigned: pratik_kamble » Unassigned
Status: Needs review » Reviewed & tested by the community

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

larowlan’s picture

+++ b/core/modules/help_topics/help_topics/core.content_structure.html.twig
@@ -20,11 +20,15 @@ top_level: true
-  <li>{% trans %}List (Options module): Stores values chosen from pre-defined lists; the values can be numbers or text{% endtrans %}</li>
-  <li>{% trans %}Reference (core system): Stores entity references{% endtrans %}</li>
+  <li>{% trans %}List (Options module): Stores values chosen from pre-defined lists, where the values can be numbers or text; see section below for more on list fields.{% endtrans %}</li>
+  <li>{% trans %}Reference (core system): Stores entity references; see section above{% endtrans %}</li>
   <li>{% trans %}Telephone (Telephone module): Stores telephone numbers{% endtrans %}</li>
-  <li>{% trans %}Text (Text module): Stores formatted and unformatted text{% endtrans %}</li>
+  <li>{% trans %}Text (Text module): Stores formatted and unformatted text; see section below for more on text fields.{% endtrans %}</li>
 </ul>
+<h2>{% trans %}What settings are available for List field types?{% endtrans %}</h2>
+<p>{% trans %}List fields associate pre-defined <em>keys</em> (or value codes) with <em>labels</em> that the user sees. For example, you might define a list field that shows the user the names of several locations, while behind the scenes a location code is stored in the database. Each list field type corresponds to one type of stored key. For example, a <em>List (integer)</em> field stores integers, while the <em>List (text)</em> field stores text strings. Once you have chosen the field type, the main setting for a list field is the <em>Allowed values</em> list, which associates the keys with the labels.{% endtrans %}</p>
+<h2>{% trans %}What types of Text fields are available?{% endtrans %}</h2>
+<p>{% trans %}There are several types of text fields, with different characteristics. Text fields can be either <em>plain</em> or <em>formatted</em>: plain text fields do not contain HTML, while formatted fields can contain HTML and are processed through <em>text filters</em> (these are provided by the core Filter module; if you have that module enabled, see the related topic below on filters for more information). Text fields can also be regular-length (with a limit of 255 characters) or <em>long</em> (with a very large character limit), and long formatted text fields can include a <em>summary</em> attribute. All possible combinations of these characteristics exist as text field types; for example, <em>Text (plain)</em> and <em>Text (formatted, long, with summary)</em> are two examples of text field types. {% endtrans %}</p>

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

andypost’s picture

  • larowlan committed c6012dc on 9.1.x
    Issue #3067614 by Scott Weston, jhodgdon, neelam_wadhwani, prabha1997,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Re #83 I see this above from @jhodgdon

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.

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!

Status: Fixed » Closed (fixed)

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