[D7] Countdown event

Hello there,

Please fix the problems found by the automated review here http://pareview.sh/pareview/httpgitdrupalorgsandboxoctoplod2106065git , most of which are mainly Drupal coding standard issues.

You also need to move your git branch from master to a version specific one as described here https://drupal.org/empty-git-master

You also need to add a README.txt to your repo describing the module and installation instructions, more information can be found here https://drupal.org/node/161085

Could you also update this page with the drupal.org specific git repository rather than the drupalcode one, eg http://git.drupal.org/sandbox/octoplod/2106065.git

Will do another review once these problems have been fixed :)
Cheers

Hey,

my thoughts:

* Empty lines at end .module file.
* You have some switch() calls, with only one single "case" each. Maybe a simple if() would suffice? Unless you plan to later extend theses cases of course.
* You add your js file in your .info file. From my understanding, that will add it globally. Maybe use drupal_add_js instead, to only load it where needed? Line 138 seems to indicate you planed to do so? If not, the comment there is unnecessary.
* JavaScript looks fine to me.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

updated the link to the git repository

Hello,

You may wish to check here for drupal coding standards related stuff.

I would also suggest you to add default values in countdown_event_block_configure when calling variable_get, because if left blank, the countdown digits are invisible.

I would add the js/css through the module file and not through the info one. Maybe someone would want to add the block on a single page and avoid loading the js/css on every other one. That's my opinion though, you may wish to keep it like this.

Oh yeah,I came across few requirement of this module, but never tried to make it as 'module' :). Here is some of my reviews:

1. Still few left to fix at http://pareview.sh/pareview/httpgitdrupalorgsandboxoctoplod2106065git
2. stylesheets[all][] = countdown_event.css - Can't we include this css only on pages where we need it?
3. window.onload = function() { in countdown_event.js may work bit different in drupal modules as behaviour. Have a look at https://drupal.org/node/304258
4. hook_init - Can be removed and including that js file can be part of countdown_event_attach() with drupal_add_js(). it may cause multiple call only when we have multiple blocks on same page. But I guess that's better than having this js in all pages?
5. We may need a .install file with hook_install all variables that we are creating.

Some future thoughts:
1. Currently this module support one counter. However it is doing it in a proper way by altering block configuration and adding it's own content. In future, we can alter the block form to provide an select box say 'Block type' => 'Countdown'. So that we can add any number of countdown blocks. But we can't use variables in that case. We need to have a table for all details.

Awesome!!! I will have a look at it again. Thanks for the updates @octoplod.

repository link changed

I reviewed this project, but didn't get far...

But first two points we do not need working code for:

(1) I don't see any need for further countdown modules. There are already some existing. But maybe you can provide more information what benefits of your module would offer in comparison.

(2) You should rename the project: Remove the word 'module'. Git cloning links take this name for the module folder what then will break naming convention (functions name not like module name). OR rename all functions (appending '_module').

Ignoring those points, talking about code, I can provide the following hints:

(3) You need an install file ('countdown_event.install'). Insert a custom 'hook_uninstall' function in order to remove variables your module creates.
Additionally you could set messages upon un-/installation.
Here's an example:

<?php

/**
 * @file
 * Install and uninstall functions.
 */

/**
 * Implements hook_install().
 */
function countdown_event_install() {
  // make translations available on un-/install.
  $t = get_t();
  // Set custom status message on installation.
  drupal_set_message($t("'countdown_event' has been succesfully installed."));
}

/**
 * Implements hook_uninstall().
 */
function countdown_event_uninstall() {
  // Delete module variables
  variable_del("countdown_event_date");
  variable_del("countdown_event_label_msg");
  variable_del("countdown_event_label_color");
  variable_del("countdown_event_background_color");
  variable_del("countdown_event_text_color");
  // make translations available on un-/install.
  $t = get_t();
  // Set custom message after uninstallation.
  drupal_set_message($t("'countdown_event' has been succesfully uninstalled."));
}

(4) Then on line 13 in the js file I found the bug preventing the module from running. The closing bracket and semicolon are missing. But then another error appears about an object not being a function.

(5) So I rewrote your JS file according to my script template. Now no errors appear. Here's the whole file:

/**
 * @file
 * This file provides the javascript for the countdown event module
 *
 * Author: Mark Collins
 */
(function($) {
  function countdown_event(context, settings) {
    $(document).ready(function() {

      // event date/time from the countdown module with timezone offset correction
      var countdownDate = (Drupal.settings.countdown_event.countdown_event_date * 1000);
      var labelMsg = (Drupal.settings.countdown_event.countdown_event_label_msg);
      var labelColor = (Drupal.settings.countdown_event.countdown_event_label_color);
      var textColor = (Drupal.settings.countdown_event.countdown_event_text_color);
      var backgroundColor = (Drupal.settings.countdown_event.countdown_event_background_color);

      // set defaults if no user preferences
      if(labelMsg === '' || labelMsg === null){
        labelMsg = 'COUNTDOWN:';
      }
      if(labelColor === '' || labelColor === null){
        labelColor = '#000000';
      }
      if(backgroundColor === ''){
        backgroundColor = '#444444';
      }
      if(textColor === '' || textColor === null){
        textColor = '#fff';
      }

      // add the custom message
      document.getElementById('label_msg').innerHTML = labelMsg;

      // add the custom label color
      var label = document.getElementById('clock_holder');
      label.style.color = labelColor;
      var units = ['countdownDays', 'countdownHrs', 'countdownMins', 'countdownSecs'];

      // add the custom background color and text color
      function customStyle(units, bgColor, txtColor) {
        for (var element in units) {
          var styledDiv = document.getElementById(units[element]);
          styledDiv.style.backgroundColor = bgColor;
          styledDiv.style.color = txtColor;
          //add class style

          //For IE
          styledDiv.setAttribute("class", 'countdownDigitBk');
          //For Most Browsers
          styledDiv.setAttribute("className", 'countdownDigitBk');
        }
      };

      //apply styling by calling function
      customStyle(units, backgroundColor, textColor);
      setInterval(function() {

        // get todays date / time
        var nowDateTime = new Date();

        // ascertain the difference between today the countdown event date / time
        var totalMilSecs = nowDateTime - countdownDate;

        // round the number down and make it positive
        var totalSecs = Math.floor(((totalMilSecs / 1000)) * -1);

        // time units expressed in secs
        var singleDay = 60 * 60 * 24;
        var singleHour = 60 * 60;
        var singleMinute = 60;
        var singleSec = 1;

        // calculte days
        var dayDiffDays = Math.floor(totalSecs / singleDay);

        // calculte hours
        var hourDiffHours = Math.floor((totalSecs / singleHour) - (dayDiffDays * 24));

        // calculte mins
        var minDiffMins = Math.floor((totalSecs / singleMinute) - ((hourDiffHours * 60) + (dayDiffDays * 24 * 60)));

        // calculte secs
        var secDiffSecs = Math.floor((totalSecs / singleSec) - ((minDiffMins * 60) + (hourDiffHours * 60 * 60) + (dayDiffDays * 60 * 60 * 24)));

        // function to add leading zero to tidy display
        var addLeadZero = function (number) {

            return ((number < 10) ? ('0' + number) : number);
        };

        var hourDiffHours = addLeadZero(hourDiffHours);
        var minDiffMins = addLeadZero(minDiffMins);
        var secDiffSecs = addLeadZero(secDiffSecs);

        var nodes = new Array();
        nodes[0] = document.getElementById('countdownDays');
        nodes[1] = document.getElementById('countdownHrs');
        nodes[2] = document.getElementById('countdownMins');
        nodes[3] = document.getElementById('countdownSecs');

        // remove existing countdown if present
        if (nodes[0].firstChild) {
            var removeNode = function (nodeName) {
                nodeName.removeChild(nodeName.childNodes[0]);
            };

            for (i = 0; i < nodes.length; i++) {
                removeNode(nodes[i]);
            }
        } //end if

        // update countdown
        var updateCountdown = function(node, timeUnit, elementId) {

            // add to dom tree
            var textNode = document.createTextNode(timeUnit);
            node.appendChild(textNode);
        };

        updateCountdown(nodes[0], dayDiffDays, 'countdownDays');
        updateCountdown(nodes[1], hourDiffHours, 'countdownHrs');
        updateCountdown(nodes[2], minDiffMins, 'countdownMins');
        updateCountdown(nodes[3], secDiffSecs, 'countdownSecs');

      //repeat every second
      }, 1000);
    });
  }

  Drupal.behaviors.countdown_event = {
    attach: function(context, settings) {
      countdown_event(context, settings);
    }
  }
})(jQuery);

At last some points I was wondering about:

(6) Since the module's block is getting it's data through PHP/JS and not HTML I don't see any problem in caching the block. So I'd alter the cache value in 'countdown_event_block_info()' from 'DRUPAL_NO_CACHE' to 'DRUPAL_CACHE_GLOBAL'.

(7) I don't see why a block title should be driven by variables. It's a fixed value. Also this would be one more variable to think about on unistallation. I didn't take this variable into the install file example.

(8) The default values added lately should be synced across the module file and the javascript file. Maybe it's even better only providing them in the module.

(9) I'm wondering why you are attaching the whole settings array to each data variable in 'countdown_event_attach()'? One of both should be enough: either attaching the array into one variable, or taking each single variable out of the array.

(10) When javascript is disabled the block is still shown. I'd think about whether to add noscript tags or (maybe better) to add the div (at least the visual part) totally by script.

(11) Last but not urgent. You should sync your project page, README.txt and 'hook_help'.

So this was my review, hope this helps you.
I will come back by time and check the progress.
Feel free to contact me if you got questions/ideas.

Cool, @octoplod update the related module details that you have mentioned in #16 in your module page (https://drupal.org/sandbox/octoplod/2106065) for future reference(can add a section called 'Similar projects' with details).

Showing as 'needs review' for me. Drupal.org has recently been update to D7 so probably something to do with that, think they're still working out some bugs

Chrome version 30.0.1599.114... Try clearing your cache?

Nice, good progress.

I've reviewed this project and am nearly satisfied.

From the points I've mentioned 10 should be adressed next, as this is fundamentaly in your scenario and could be causing trouble/confusion. The rest is more on the extreme side (optimisation, documentation and so on).

Here some small things I can provide you snippets today:

(12) Provide a config link
In your .info file we could set the configuration link for your block:

configure = admin/structure/block/manage/countdown_event/countdown_event/configure

This way the configuration can be reached directly from the module page (when installed & enabled).

(13) Config link in install message
That configuration link could also be part of the recently added installation message:

drupal_set_message($t("'countdown_event' has been successfully installed. You may now <a href=\"/admin/structure/block/manage/custom/main/configure\">configure the countdown event block</a>."));

Now we don't even need to scroll after installation.

(14) hook_help() and README.txt
Providing a README.txt file is recommended for many reasons. Syncing this file with hook_help and the project page can be painful. So i recommend you to read the content from README.txt into hook_help:

/**
 * Implements hook_help().
 */
function countdown_event_help($path, $arg) {
  if ($path == 'admin/help#countdown_event') {
    // Return content of the module README.txt.
    return check_markup(file_get_contents(dirname(__FILE__) . "/README.txt"));
  }
}

Handy little module. I could see a lot of uses for this. I installed and configured this on a very "heavy" drupal site that has over 100 modules. It installed via Drush with no issues. Looking through the code it was clean and appeared to have all proper formatting and documentation.

I'll be keeping this in my "back pocket" for the future.

No problems found, I agree with R2-D8.

Thanks for your contribution, octoplod!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.