CVS edit link for joshthegeek

I want to maintain a module that puts a JavaScript based game of TicTacToe on a page of your choice. I am still working on it. The module's configuration is at admin/settings/tictactoe. On that page, it asks for the URL to put the game on. (It uses url() to validate it.) It also asks for the title of that page, which it passes through t().
My other module is called Admin Menu Hider, which does what it says. It asks for a key and whether or not it should hide it after load. The settings in puts into the JavaScript settings. The JavaScript waits for you to press the key (by default `), and then uses jQuery's slideToggle on the admin menu. It bumps the top margin back to 0px when the menu is hidden. Admin Menu Hider already exists, but the last commit was 1 year ago, and my issue at http://drupal.org/node/910576 is almost a month old with no response. I had developed my module before finding this, but I have not used Admin Menu Dropdown. AMD has occasional incompatibility issues, but my module just adjusts the CSS positioning. AMD has an open issue for merging AMD and admin menu at http://drupal.org/node/226012, and sun, the admin_menu maintainer, had replied.

Comments

Josh The Geek’s picture

Title: joshthegeek [joshthegeek] » Contribution
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new1.46 KB

I attached my Admin Menu Hider module.
TicTacToe is still a work in progress.

Josh The Geek’s picture

Title: Contribution » joshthegeek [joshthegeek]

Oops. :)
Changed the title back. Sorry!

avpaderno’s picture

Issue tags: +Module review

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

avpaderno’s picture

Status: Needs review » Needs work
  • The points reported in this review are not in order of importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.
  1. The module doesn't implement hook_uninstall(), or doesn't implement it to remove the Drupal variables it defines.
  2. Hook implementation comments should be like the following one:
    /**
     * Implements hook_menu().
     */
    

    As reported in Documenting hook implementations:

    If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

    /**
     * Implements hook_help().
     */
    function blog_help($section) {
      // ...
    }
    

    This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

    In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

    /**
     * Implements hook_form_FORM_ID_alter() for node_type_form().
     */
    function mymodule_form_node_type_form_alter(&$form, &$form_state) {
      // ...
    }
    

    This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

  3. The proposed module duplicates the work done from an existing project, which is what we try to avoid. It's not clear to me why the same functionality could not be incorporated into Admin Menu, then. Did you open a feature request for the existing project?
Josh The Geek’s picture

Title: joshthegeek [joshthegeek] » Josh The Geek [joshthegeek]
Status: Needs work » Needs review
StatusFileSize
new2.12 KB

I fixed the hook commenting errors and added the hook_uninstall. (I attached the new version.)
I posted issue #910576: Offering to maintain Administration Menu Dropdown about my basic port to Drupal 6 over a month ago with no reply.

Josh The Geek’s picture

I have abandoned the TicTacToe module. It works, but the JavaScript has a bug in it that can't be fixed without removing basic functionality. I just won't fix it because no one really cares about TicTacToe.

Josh The Geek’s picture

Anyone here?
(BTW: The post #910576: Offering to maintain Administration Menu Dropdown is almost two months old with no reply.)

brianV’s picture

Status: Needs review » Needs work

Josh,

Perhaps your best path is to try to take over maintainership of Administration Menu Dropdown if your module closely replicates it's functionality, and it appears to be abandoned.

see http://drupal.org/node/251466.

There is also an issue to actually merge that functionality directly into admin_menu:

http://drupal.org/node/226012

To review your code:

  1. Please remove the .svn directory and other hidden files.
  2. Please remove the following from your .info file:
    version = "6.x-1.1"
    core = "6.x"
    datestamp = "1284319175"
  3. Please add @file docblocks to your .install and .module files. http://drupal.org/node/1354#files
  4. Typo:
    /**
     * Implementats hook_init().
     */
    
Josh The Geek’s picture

Status: Needs work » Needs review
StatusFileSize
new4.02 KB

I fixed all of these. (See attached)
I posted #910576-1: Offering to maintain Administration Menu Dropdown to ask for maintainership, and contacted Shawn Conn via his account page.

brianV’s picture

Alright. At this point, your module looks good on it's own merits. However, as a duplicate, it makes sense to wait on this application to see if there is a response from the Admin Dropdown module maintainer.

If not, we will approve you as a co-maintainer, and you should be able to release your code as the D6 port. Please bump this in two weeks - things tend to get lost in this issue queue.

shawn conn’s picture

I'm the creator and my free time hasn't allowed me as much time with Drupal module maintenance as I would like. I'm glad to hear someone has found it useful enough to keep up maintenance. Per Josh's request, I'm giving him CVS access.

brianV’s picture

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

Status: Reviewed & tested by the community » Fixed

I am approving the CVS account as maintainer of Administration Menu Dropdown.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Josh The Geek’s picture

Status: Fixed » Needs review

Great! Thank you all so much.

Josh The Geek’s picture

Status: Needs review » Fixed

What the heck? Changed the status by accident.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

avpaderno’s picture

Component: Miscellaneous » co-maintainer application
Issue summary: View changes
Status: Closed (fixed) » Fixed

Status: Fixed » Closed (fixed)

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