Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Akanksha92 created an issue. See original summary.

Akanksha92’s picture

Assigned: Akanksha92 » Unassigned
Status: Active » Needs review
FileSize
5.12 KB

According to "https://www.drupal.org/docs/develop/documenting-your-project/readme-temp..." README files should be as per Drupal standard. Updating the File. Please Review

Eitisha’s picture

The patch applied cleanly. But it has some warnings. Please look into it. Attached is the screenshot.

Status: Needs review » Needs work

The last submitted patch, 2: updating_readme-2933484-1.patch, failed testing. View results

bhanuprakashnani’s picture

Assigned: Unassigned » bhanuprakashnani
Status: Needs work » Needs review
FileSize
5.46 KB

Please review and mention if any more changes are to be made. I have changed it as per the Drupal README standards. Thank you.

daniel.nitsche’s picture

Title: Updating readme file as per standards » Updating README file to use structure from documentation standards
Issue summary: View changes
daniel.nitsche’s picture

Status: Needs review » Needs work

Thanks for this, because I think the README needs some work, and hasn't been updated in an while.

I've updated the issue summary, because I think the README needs to be rewritten in places, but that is outside the scope of the original issue. Where the text has been modified/updated in a patch, and I believe it needs work, I've mentioned that below.

I see a few issues with the patch in #5:

1. There is a line break after "Administer > Configuration > Search and metadata > URL aliases" which is causing "Delete aliases" to be treated as a quote (example here: https://codepen.io/anon/pen/BxvPLr)
2. This line has been removed: "If you are developing for this module, have a look at pathauto.api.php." -- as per the new issue summary, I think we should avoid a full rewrite for now, and just aim for a restructure.
3. " Navigate to Administration > Extend and enable the Sign for pathauto module and its dependencies." -- I don't think this is necessary based on the guidelines. If we want to keep this, it should read: "Navigate to Administration > Extend and enable the Pathauto module."
4. Rename "Notice" back to "Notices" or come up with a better name (Notes?)
5. "If the module is not shown in the list try deleting the module and try cloning it again. or else try clearing the cache, and then try installing it." -- I think this should be removed as it's not specific to this module. If we want a troubleshooting section, I'd move the last two points from "Notices" in here. We could also rewrite those last two points as questions, and rename the section "FAQs".
6. "For external links, you might want to consider the Path Redirect or Global Redirect modules, which allow you to set forwarding either per item or across the site to your aliased URLs." -- this should be moved to the "Recommend Modules" section

Vidushi Mehta’s picture

Status: Needs work » Needs review
FileSize
5.59 KB
2.85 KB

Added a patch according to the #7 above mentioned points with the interdiff.

alonaoneill’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch for spelling and grammar. Formatting aligns to Drupal documentation standards. Everything looks good. Patch applied.
Marking as RTBCed.
Thank you for working on module!

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/README.md
    @@ -11,20 +22,60 @@ central settings path for site administrators.
    +This module requires the following module:
    +
    + * Token - https://www.drupal.org/project/token
    + * CTools (8.x-1.x only) - https://www.drupal.org/project/ctools
    +
    +
    +RECOMMENDED MODULES
    

    If we're touching this, lets make some more improvements.

    The 8.x-1.x part isn't necessary because this is the readme for the 8.x-1.x version.

  2. +++ b/README.md
    @@ -11,20 +22,60 @@ central settings path for site administrators.
    + * For external links, you might want to consider the Path Redirect or Global Redirect modules, which allow you to set forwarding either per item or across the site to your aliased URLs.
    

    The functionality of these modules has been merged into redirect.module in D8, so we can drop this.

  3. +++ b/README.md
    @@ -77,7 +123,7 @@ Other suggestions and patches contributed by the Drupal community.
     
     Current maintainers:
     
    -- Dave Reid - http://www.davereid.net
    
    -- Dave Reid - http://www.davereid.net
    -- Greg Knaddison - http://www.knaddison.com
    
    -- Greg Knaddison - http://www.knaddison.com
    -- Mike Ryan - http://mikeryan.name
    
    -- Mike Ryan - http://mikeryan.name
    -- Frederik 'Freso' S. Olesen - http://freso.dk
    
    -- Frederik 'Freso' S. Olesen - http://freso.dk
    + * Dave Reid - http://www.davereid.net
    + * Greg Knaddison - http://www.knaddison.com
    + * Mike Ryan - http://mikeryan.name
    + * Frederik 'Freso' S. Olesen - http://freso.dk
    

    Most of these haven't maintained this module in a very long time?

    Lets keep dave and add me?

shubham.prakash’s picture

Status: Needs work » Needs review
FileSize
5.35 KB

This patch will fix the mentioned issue.

ravi.shankar’s picture

Status: Needs review » Reviewed & tested by the community
Berdir’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, don't forget to post interdiffs to make it easier to review changes.

  • Berdir committed f4f49ce on 8.x-1.x authored by shubham.prakash
    Issue #2933484 by Vidushi Mehta, Akanksha92, bhanuprakashnani, shubham....

Status: Fixed » Closed (fixed)

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