CSS Menu

Sandbox project: http://drupal.org/sandbox/AtulBhosale/1289456

Is this different enough from http://drupal.org/project/nice_menus that you couldn't collaborate? If so, please post an explanation of the differences to the project page and move this back to "needs review." If they are similar enough, you should contribute to the existing project rather than creating a new project.

From a quick glance, it appears that the only difference between this and the 'nice menus' module is the in-admin configuration of the menu css attributes. Am I correct?

If this is the case, would you consider approaching the maintainer of the 'Nice Menus' project with a patch to add the in-admin configuration of css styles to that project, rather than make end-users choose between two modules that otherwise do the same thing? (Given the choice, many end-users will choose the older, more established project ... and thus not be able to benefit from your work in this module.)

Also, can you please shrink the screenshot image which is included on your sandbox project page? It breaks the layout of the page, and hides the repository links that many of us depend on in order to perform code reviews.

If you do choose to move forward with this module, rather than simply adding the configuration enhancements to the existing 'nice menus' module, then Coder reports one trivial issue which you should fix up:

cssmenu.install
Line -1: @file block missing ( http://drupal.org/node/1354#files )

Also, it appears that there are a number of cases where unsanitized data could be output to the screen, thus resulting in vulnerability to XSS attacks. Please ensure that any variable text (menu titles or descriptions, for example) are passed through a sanitization filter such as check_plain, filter_xss, or filter_xss_admin before being output to the screen.

For vulnerability I added "check_plain" for description...

I see no new code in your sandbox repository since September 24th.

A few more comments ...

1. Your cssmenu.info file needs a few more lines ... for example, the core version of Drupal that this is supposed to work with.

2. Can you comment on why you have multiple css files, with the same definitions appearing in multiple files? Unless there's a valid reason for this, the files should be cleaned up.

3. colorpicker.js / colorpicker images: Is this a 'stock' build of colorpicker? Generally speaking, the preference is to not include 3rd party libraries in your Drupal module ... rather, provide the user with instructions on how to obtain and install the 3rd party library separately. Consider using the Libraries API, which will i) enable multiple modules to use the same library, ii) simplify upgrades to the library code itself, and iii) avoid any potential issues with multiple versions of the same 3rd party library being installed by different modules.

4. custom.js: If this is part of colorpicker, than point 3 above applies. If it is your own code, then check out http://drupal.org/node/304255 and http://drupal.org/node/304258 for information on the 'Drupal' way with regards to custom javascript in Drupal modules.

I saw your review comment for CSS Menu module (http://drupal.org/node/1289484#comment-5121812).
In CSS Menu module color picker is used from http://www.eyecon.ro/colorpicker/ site.
When I tried to find plugin on JQuery.com I found http://plugins.jquery.com/project/color_picker plugin which is written by same author ie. Stefan Petre.

Also the following links from JQuery plugin page points to http://www.eyecon.ro/colorpicker/
Read documentation
Try out a demonstration

But download is not available on JQuery.com.

Please keep your questions within the review thread, rather than email ... this way, others can learn from the discussion as well.

There is no requirement that a plugin needs to be from jquery.com ... you can add instructions in your README.txt file asking that users download the colorpicker plugin from the eyecon.ro site, and use hook_requirements to verify that it is installed.

I'd suggest looking at http://drupal.org/project/jquery_colorpicker to see how they did things ... you might even be able to create a dependency on that module (if it makes sense), and skip needing the instructions.

Libraries API is certainly the right way to go! :)

image width and height updatec

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• Comments: there should be a space after "//".
  

  cssmenu.admin.inc:377:  //clear css cache, as we have configure style for menu
  

• ./cssmenu.admin.inc: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function cssmenu_overview_form($type, $edit = array()) {
  

• ./cssmenu.install: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function cssmenu_schema() {
  

• ./cssmenu.admin.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function _css_menu_confirm_delete(&$form_state, $mid) {
  function _css_menu_confirm_delete_submit($form, &$form_state) {
  

• ./cssmenu.admin.inc: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  52- *  Not used.
  54- *  An associated array of drawer menu block details used in block edit operation
  318- *  Not used.
  320- *  Get form values form submissions.
  386- */
  

• ./cssmenu.module: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  139- *  Menu id to reder block
  141- * @ingroup
  156- *  Menu id to reder block
  158- * @ingroup
  186- *  An array of block title and menu links.
  198- *  Commoa seperated parameters key to generate preview
  200- *  Commoa seperated parameters value to generate preview
  202- *  Theme HTML to render preview
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  cssmenu.admin.inc:31:  if ( (isset($_POST['op']) && $_POST['op'] == t('Delete')) || !empty($_POST['confirm']) ) {
  cssmenu.admin.inc:345:  if ( 0 == $mid ) {
  cssmenu.module:85:    if ( is_array($menu_details) ) {
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  README.txt:1:$Id$
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

You still have a few minor issues being flagged by the automated code review functions, I also noticed a few style misnomers during a manual review.

Automated Review

• ./cssmenu.admin.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  55- *  Not used.
  58- *  An associated array of drawer menu block details used in block edit operation
  323- *  Not used.
  326- *  Get form values form submissions.
  391- */
  

• ./cssmenu.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  145- *  Menu id to reder block
  148- *
  165- *  An array of block title and menu links
  168- * @ingroup
  196- *  A HTML of menu links.
  208- *  Commoa seperated parameters key to generate preview
  211- *  Commoa seperated parameters value to generate preview
  214- *  Theme HTML to render preview
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./css/box_default.css ./css/cssmenu.admin.css ./cssmenu.admin.inc ./cssmenu.info ./cssmenu.install ./cssmenu.module ./js/cssmenu.js ./README.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual Review

Had a quick glance over your code and there are however a couple of issues I feel are in need of addressing.

• Remove HTML code sections outside your t() functions as this can create problems for translators. Check cssmenu.install.
• Your CSS file does not conform to the Drupal CSS Coding Standards with particular attention to alphabetizing attributes and commenting conventions.
• The @charset "UTF-8"; in your CSS files is not required.
• Rather than using $(document).ready() in your JavScript you should possibly employ Drupal's native method using Drupal.behaviors.

The automated review scripts are still flagging you on quite a number of points.

see http://ventral.org/pareview/httpgitdrupalorgsandboxatulbhosale1289456git

The updated reviews scripts are still flagging you on quite a number of issues, see http://ventral.org/pareview/httpgitdrupalorgsandboxatulbhosale1289456git...

Most of the issues where to do with spacing within files where you have used tabs rather than spaces.

You also seem to be missing parameter types within your documentation blocks, as you have previously noted, I would say to you to add these in anyway as previous Coder reviews wont flag them as incorrect even if the standard has only been adopted for D8.

To err on the side of caution and in an attempt to make your module a little more forward compatible I would say to you to attempt to implement some of the Drupal 7/8 styles/standards with regards to commenting. If this method is employed then your reviews will surely go a lot smoother and your documentation will be top class.

I reran http://ventral.org/pareview/httpgitdrupalorgsandboxatulbhosale1289456git... to get the latest code, and it is still showing a few errors.

Switching back to needs review as there are only a few issues thrown by pareview.

You have not listed any reviews of other projects in your issue summary, as strongly recommended in http://drupal.org/node/1011698
See also #1410826: [META] Review bonus.

Live demo added

Would it not be nicer having HTML-Code in Theme functions?
_cssmenu_overview_tree_form($tree)

Misspelled comma ...

 * @param string $key
 *   Commoa seperated parameters key to generate preview.
 * @param string $value
 *   Commoa seperated parameters value to generate preview.

I reviewed the module and found no security or best practice problems. I don't see any objections for making it a "full project", even so I did not check if it is actually working.

1. "Allows users to cofigure menu and style" IMO this description could be better
2. Add a general module description to your readme, currently it only contains dependencies and installation instructions
3. variable_get('file_directory_path', '');this will fail if no variable for this setting is set yet, use file_directory_path() instead

4. /**
    * Implements hook_load().
    *  To load CSS menu block details.
    */

   follow coding standarts and make a new line between short instruction and long function description, keep the descriptions the 'does' way (please correct this everywhere):

   /**
    * Implements hook_load().
    *
    * Loads CSS menu block details.
    */

5. $data = $block_data = $menu_details = array(); you don't have to initialize $data here, it'll be overwritten on the next line (also I think this syntax is quite ugly)
6. 'CSS menu ( ' . $list->title . ' )' keep thing translatable: t('CSS menu (@title)', array('@title' => $list->title)
7. End all your comments with fullstop (or !, ?)
8. _cssmenu_get_plain_text(), you should give that function a proper name, first thought you were duplicating check_plain() here
9. $t('<strong>CSS... remember to put as few HTML as possible into t(), this will make translators life much easier
10. Still many strings in your admin.inc not wrapped in t() (eg. "There are no css menu yet.", "Vertical right fly"...)
11. your using check_markup() in line 313 what makes no sense for me, why do you want to check non-user-generated text here ?
12. why do you craft your own confirmation step, why don't you make use of confirm_form()?

regards

removing review bonus tag, you can add it again after you've done 3 more reviews (remember to pick old issues).
thanks

Yep sorry, I first thought your not using it, but forgot to remove the point afterwards

Setting this up on a D6 install the CSS-settings I configured didn't do anything. The menus were just flat lists without effects or any special CSS.

Am I missing something? The configuration page with colorpicking worked fine, the block was created and I've tried it both in a top or left menu position (using Garland as theme).

Needless to say the Preview-link didn't work. Oddly enough it seems to have a broken edit-path and sends me to admin/build/cssmenu/edit/void(0); which gives me an empty CSS Menu form.

Note that automated review tools are pretty dumb about finding security issues - these issues should just point you to the line where a security issue is possible - this does not mean that it actually is a security issue!

As we now know it's a false positive - just ignore it.

I am reviewing this module now.

Very nice module. It seems to work well. Most of the problems I found were documentation-related.

README.txt

In "Download from colorpicker.zip from http://www.eyecon.ro/colorpicker", remove "from".

There are some more steps I would recommend adding:

- Add the step of enabling the module

- Add instructions for once the module is installed:

-- Go to admin/build/cssmenu/add to configure a new CSS menu.

-- After saving, go to admin/build/block to configure the block that has been automatically created for the menu.

There are a number of things that should be fixed in the function documentation. You should follow the guide here at http://drupal.org/node/1354.

I will also list fixes that document doesn't cover.

cssmenu.admin.inc

Some fixes to user-facing text:

"There are no css menu yet." -> "There are no CSS menus yet."
"Fail to save" -> "Failed to save"
"Fail to update" -> "Failed to update"
"The CSS menu %title have been deleted." -> "The CSS menu %title has been deleted."

Documentation fixes:
"An associated array" -> "An associative array"

I am told it is preferable to use the Form API #attached property (more info at http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_pro...) to drupal_add_js() and drupal_add_css()

cssmenu.module

"This is a simple module that enables the site to have fully configurable CSS menu for site." -> Should say "menus"

The function documentation should start with a 3rd-person verb, for example:
- Doc for cssmenu_load, "Load CSS menu block details." has extra space in front and should be "loads" instead of "load".
- Doc for cssmenu_block "Generate a CSS menu block." should say "Generates"

There are a number of misspellings: "reder" should be "render", "seperated" should be "separated", "alpha/numeric" should be "alphanumeric".

The @ingroup line should be removed, since it should only be used when it lists what groups the function belongs to.

For _cssmenu_preview:
- The first doc line should start with a capital letter and a verb (e.g., "Renders the CSS block...").
- It's hard to understand what the parameters are. Maybe they should say something like
-- Comma-separated list of property keys for the menu
-- Comma-separated list of property values for the menu

For _cssmenu_get_clean_string:
- The documentation "Returns a alpha/numeric sting in lower case" should be "Returns an alphanumeric string in lowercase"
- "String to remove non alpha/numeric characters." should be "String to remove non-alphanumeric characters from."

cssmenu.js

I could be wrong, but I think your JavaScript should have
attach: function (context, settings) {}
to contain the code that isn't function definitions.

I do automated project review, and there some minor coding style errors.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/./test_candidate/cssmenu.admin.inc:
+327: [critical] Potential problem: when FAPI element '#type' is set to 'markup' (default), '#value' only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

Status Messages:
Coder found 2 projects, 2 files, 1 critical warnings, 0 warnings were flagged to be ignored

FILE: ...review/sites/all/modules/pareview_temp/test_candidate/cssmenu.admin.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
415 | ERROR | Function comment short description must be on a single line
431 | ERROR | Function comment short description must end with a full stop
--------------------------------------------------------------------------------

Sorry guys. ;)

Sorry for the delay. Make sure to review more project applications and get a new review bonus and I will get this finished immediately.

Reviews of other projects added

manual review:

1. cssmenu_load(): I think you have implemented hook_load() by accident here, as you don't do anything with nodes here. Rename that menu autoloader to avoid confusion and unexpected behavior.
2. _cssmenu_preview(): no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';
3. cssmenu_form(): style information for your form items should be palced in a dedicated CSS file.
4. cssmenu_form_submit(): use drupal_write_record() instead of the manual insert/update queries and you will get DB schema validation for free.

But that are no blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution, vaibhavjain! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.