Had a look over the module over the weekend and I found some things inside the code that might be good to look over:

  • There is a typo in the info file "durin" instead of "during"
  • The module contains links.menu.yml file that is setting a link under the Structure menu item - I guess this was not intended since the link already exists under the Configuration menu item
  • The links.task.yml file is putting a link inside the cores configuration routes, because of that I propose to change the route link to "Ignore" since we already know this belongs to the configuration part
  • I also propose to set the modules package to "Configuration" so it belongs to the same group as other configuration modules. I've checked Configuration Read-only mode module has the same

Out of the list, I guess only the typo part is the only thing that needs to be fixed and maybe the duplicate menu entries, the rest of the points are more UX and standardisation oriented. Let me know your thoughts.

CommentFileSizeAuthor
#2 2845729-2.patch1.58 KBjzavrl
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jzavrl created an issue. See original summary.

jzavrl’s picture

Here is also a patch of proposed changes.

jzavrl’s picture

Status: Active » Needs review
tlyngej’s picture

Status: Needs review » Reviewed & tested by the community
  1. Good catch!
  2. This was a leftover from a very early version I had, and yes, it should be removed.
  3. The change of the title makes sense, since the user is already in the context of configuration.
  4. Sure. Let's follow what the rest of the similar modules does. This change makes sense.

Lovely work!

Marking of as RTBC and will commit soon.

Thanks a lot.

  • tlyngej committed d0fe3dd on 8.x-1.x authored by jzavrl
    Issue #2845729 by jzavrl: Typo in code and some standardisation...
tlyngej’s picture

Status: Reviewed & tested by the community » Fixed
tlyngej’s picture

Status: Fixed » Closed (fixed)