The release notes for Drupal 8.1.0 indicates an " Improved site administration experience: * Improved admin/help page to be more flexible and list tours on it."

The Help text standard (for core and contrib) gives a detailed account of how to do this the 'drupal way'.

I've been working on this for a few other modules and should be able to make quick work of getting this in for Video Embed Field, so I've assigned this issue to myself to work on. The way I've done it elsewhere is to implement each aspect (1-4) as a separate patch/issue. We'll see how that approach works here.

This is a followup to #2716941: Add README file which is a small stepping stone to improving HELP documentation for this module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dbt102 created an issue. See original summary.

dbt102’s picture

Issue summary: View changes
dbt102’s picture

So, I mentioned above I'd work thru items 1-4. I'll just list them here and comment on each as kind of my review checklist before submitting the patch...

<1. Short description of what the module does. It is displayed on the Extend or Modules page (in Drupal 8 or 7). It is the only texts users will see if the module is not enabled yet.

^-- N/A ... this is OK, the description already exists and looks good to me

2. Description on links are displayed with the links on the Configuration and Structure pages and invite users to do something.

^-- N/A ...there is really no Configuration and Structure pages per say, for this module, so this section does not apply.

<3. Explanations on the administration pages. Ideally this should not be needed, but if they do they are short and do not duplicate the help page.

^-- N/A ... The existing explaination/descriptions on the permisions admin pages at /admin/people/permissions#module-video_embed_field look good to me.

4. Help page displayed by the Help module with three sections: What does the module do, what can users do with it, and a link to the online documentation here on drupal.org. This hook_help() text is in the my_module.module file.

^-- This is the scope of the patch #4 included below. Also, I added an "about" type documentation page as part of this update, just to kind of orient new users to what this module does before diving into the more detailed config page that follows

This checklist reviews the new HELP work added for the main Video Embed Field module. The two additional sub-modules (Video Embed Media and Video Embed WYSIWYG
) will be addressed as separate issues.

NOTE: Once these HELP components are in place we anticipate adding on TOUR functionality as a followup issue.

dbt102’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: Add_Video_Embed_Field_HELP-2716967-4.patch, failed testing.

The last submitted patch, 4: Add_Video_Embed_Field_HELP-2716967-4.patch, failed testing.

The last submitted patch, 4: Add_Video_Embed_Field_HELP-2716967-4.patch, failed testing.

Sam152’s picture

These fails are likely in HEAD. media entity got a new release which likely has failed the upgrade path test.

The last submitted patch, 4: Add_Video_Embed_Field_HELP-2716967-4.patch, failed testing.

Sam152’s picture

Not sure about copying the providers into the hook help? A link to the project page seems like enough? I don't fancy the idea of duplicating information between the README, project page and hook help. Lets keep hook help entirely non-technical. I don't see it as a tool for developers.

dbt102’s picture

Thanks @sam152,

For what its worth, I tested and retested to make sure the patch applied correctly at least on my local.

Thanks for the work on a great module!

dbt102’s picture

You are right of course, but we have a couple customers that will use this HELP a lot. So I thought I'd throw it in.

Just trying to make it easy for the very novice user to see that there are many options they can come back to dev's to get implemented.

dbt102’s picture

Assigned: dbt102 » Unassigned

Unassigning the task from myself so others won't shy away from testing this patch

Sam152’s picture

Sam152’s picture

Status: Needs review » Needs work
  1. +++ b/video_embed_field.module
    @@ -2,8 +2,31 @@
    - * Module file for video_embed_field.
    + * This is the module file for video_embed_field.
    

    Unrelated change?

  2. +++ b/video_embed_field.module
    @@ -2,8 +2,31 @@
    +      $output .= '<dl>';
    

    Where is this closed?

  3. +++ b/video_embed_field.module
    @@ -2,8 +2,31 @@
    +      $output .= '<dd>' . t('The <em>settings</em> and the <em>display</em> of the video embed field can be configured separately. See the <a href=":field_ui">Field UI help</a> for more information on how to manage fields and their display.', array(':field_ui' => (\Drupal::moduleHandler()->moduleExists('field_ui')) ? \Drupal::url('help.page', array('name' => 'field_ui')) : '#')) . '</dd>';
    

    This doesn't seem like it adds much value?

  4. +++ b/video_embed_field.module
    @@ -2,8 +2,31 @@
    +      $output .= '<dd>' . t(' <a href="https://www.drupal.org/project/video_embed_aol">AOL</a>, <a href="https://www.drupal.org/project/video_embed_brightcove">Brightcove</a>, <a href="https://www.drupal.org/project/video_embed_dailymotion">Dailymotion</a>, <a href="https://www.drupal.org/project/video_embed_dreambroker">Dreambroker</a>, <a href="https://www.drupal.org/project/video_embed_facebook">Facebook</a>, <a href="https://www.drupal.org/project/video_embed_instagram">Instagram</a>, <a href="https://www.drupal.org/project/video_embed_metacafe">Metacafe</a>, <a href="https://www.drupal.org/project/video_embed_myspace">MySpace</a>, <a href="https://www.drupal.org/project/video_embed_rutube">Rutube</a>, <a href="https://www.drupal.org/project/video_embed_ted">Ted</a>, <a href="https://www.drupal.org/project/video_embed_vidyard">Vidyard</a>, <a href="https://www.drupal.org/project/video_embed_field">Vimeo (packaged with the module)</a>, <a href="https://www.drupal.org/project/video_embed_vine">Vine</a>, <a href="https://www.drupal.org/project/video_embed_wistia">Wistia</a>, <a href="https://www.drupal.org/project/video_embed_youku">Youku</a>, <a href="https://www.drupal.org/project/video_embed_field">YouTube (packaged with the module)</a>') .'</dd>';
    

    I don't feel like maintaining this twice is sane.

  5. +++ b/video_embed_field.module
    @@ -2,8 +2,31 @@
    +      $output .= '<dd>' . t('Any (or all) of these additional plugins to the Video Embed field module adds the ability to embed the additional video type in a manner similar to those the Youtube and Vimeo types included in the main video_embed_field module') . '</dd>';
    

    Video Embed Field case inconsistency.

dbt102’s picture

Assigned: Unassigned » dbt102

Thanks for the great feedback sam152

assigning this back to myself to work on

dbt102’s picture

notes:

1. Unrelated change? --> yep, changed back
2. Where is this closed? --> oops ... added closing statement
3. This doesn't seem like it adds much value? --> agreed
4. I don't feel like maintaining this twice is sane. --> no problem ... removed
5. Video Embed Field case inconsistency. --> yep, the proper noun is "Video Embed Field" ... fixed

dbt102’s picture

Assigned: dbt102 » Unassigned
Status: Needs work » Needs review
dbt102’s picture

just checking in to see if there are any other changes you want to see made for this. At this point, for D8 modules, I think any HELP is better than none at all.

Sam152’s picture

Sorry for the hold up. I've made some necasary changes to this:

  1. Link to the top level docs page.
  2. Removed short-hand arrays, remove deprecated method calls, move links into their own array.
  3. Updated some wording.
  4. Made the content only link off to the online docs pages or other help pages.

Sam152’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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