Accordion

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxalexwang1826668git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Manual Review :

1) Please add README.TXT which tell the module description and configuration.
2) Remove project & version tag from .info file as they will automatically added after module contribution.
3) Rename the alu_accordion_select_nodes function with your module name
4) Move your code repository from branch to 7.x-1.0 branch and then delete the master branch.

you are working in the "master" branch in git. You should really be working in a version specific branch.

Please follow Drupal coding standards.

Hi Alex, Nice module.

Please find comment below for your reference.

1. Description:

You need to paste your git checkout command here like
git clone http://git.drupal.org/sandbox//.git your_project_name

Better to have more description about your project

2. .info

Please to have description and more details

3. code

well organized.
Better not to include not necessary things.
255 // cache_set('accordion-'. $level . '-' . $argument, $output, 'cache');
256 // }else{
257 // $output = cache_get('accordion-'. $level . '-' . $argument);
258 // $output = $output->data;
259 // }

Better to get a review bonus and Good to go

# The code has some comments, in case you don't need these, please remove these.
# Please create a better project page. Please refer here for creating great project pages.

I've chosen to do a usability review because of your wonderful addition of a demo site (thanks!). A little bit of code review is mixed in for good measure.

First of all, I think the idea is a good one, a kind of sitemap or content inventory based on taxonomy. Useful, especially for ecommerce.

Your demo site confuses me a little bit. Lets look at your terms N and T. Both have subterms and some nodes. N has a node directly tagged, and T only has a node tagged with subterm B. However, both nodes are placed identically when visible. In order to make it clear what node belong to which terms, I would show nodes related to a term at the same level of indentation as the term they are tagged with. Alternatively, you could show nodes directly after the terms they are tagged with instead of at the bottom of the section. Since you have a css file with default styling, I would argue taking care of this visual confusion is in the scope of your module. Also, please remove commented out css from your ajax.css .

I was also confused why I got a 'loading ajax' animation for simply collapsing down a category, but then I realized that you are loading the whole category again-- which is also why everything appears clickable. You might consider not reloading sub categories on repeated visits to the same place. It's unlikely new nodes will have sprung up in between a couple of clicks. It would require a more complex module, but it would feel faster to revisit a branch you have opened before without reloading everything.

This module would be particularly good in combination with my module Taxonomy Set Lineage #1901802: Taxonomy Set Lineage, so that nodes in a sub category would also show up in the parent category. Shirts > T-Shirts > Band T-Shirts etc... maybe for shirts you want to show everything.

You also seem to have only have thought about doing the job of providing a no js fallback for accesibility. These days you can expect even screen readers to have javascript, but search engine crawlers mostly don't. There is no way to drill down without js turned on and one is sent along to a page with "This is some content delivered via a page load." It looks like you got that all out of the ajax example module or something and need to follow through with a no js version, or just don't pretend to have one. In that case, take out the if ($type == 'ajax') { conditional that wraps your function.

Also if I don't configure the block, but just activate it, I get a notice in the front end ... Notice: Trying to get property of non-object in _accordion_get_taxonomy_menu() (line 167 of ...sites/all/modules/accordion/accordion.module). Please test to see if the user has picked a vocabulary, and if not, bail out or set a message that they should. You have a default taxonomy name of 'product_category' which I think is unlikely to be on most sites. Instead of providing a default name for that variable, you have to test if it returns something that exists in the system.

I also am worried about your namespace here. "Accordion" is pretty broad and non descriptive of what this module actually does. That might be what you call this block on your site, but I would suggest "Taxonomy Ajax Accordion" might give people a much more specific idea of what they are getting when they download. You need taxonomy in the name probably to have folks find your module, in my opinion.

Lastly, as others have noted, there are some commented out sections in the code, and you should get rid of these.

forgot to set to needs work.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

add demo site