Sass Variables

Hi,nick_schuch

When i clone your module it give me only README.txt file nothing else.

Mayank Kamothi

Hello Nick,

I tested your module and here is my review:

About the code:

• Your module is not very large, so imho comments like /*********** CONSTANTS ***********/ could be removed.
• I personally don't like it when modules don't clean up her own variables. I think it is nice practice to clean schema AND variables. Please add an .install file with an hook_uninstall which deletes the vraibles you introduce to the system.
• The generated variables are concatinated in one line. I think a "\n" (newline) would be more developer friendly.
• Because I do not know all sources of hacking and possible injection types, a sanitizing of the variable could be usefull.

Usage:

• When no theme is installed or defined (not sure) a big red warning is displayed with a PHP foreach error. Because theme_get_settings returns NULL, when nothing is found. Please add a condition here which displays a helpfull message and optionally, removes the rest of the $form from beeing rendered, becuase of no use.
• Generally - after I followed your instructions correctly - it would be nice if there would be a text which tells me to set variables via settings[sass_variables][foobar] first, because in the enabled theme where no found.

On the settings page after pressing the button "Save configuration & rebuild mixin", a warning comes out. The reason is that there is an invalid variable $sass_variables on the line 124 of sass_variables.module file.

Code looks good. I have a few comments though:

-You should remove the master branch of your module. Take a look at Moving from a master to a major version branch.
-Some of the lines in your readme file exceed 80 characters.
-In the implementation of hook menu, title and description should be passed trough t function.
- on line 74 should be something like this instead '#title' => t($description . " (!id)" ,array('!id' => $id)),

Cheers,

Troels

@trogels: t() Functions should only contain true strings, no variables. Your 4th suggestion is wrong.

Loved that you included a YouTube video in the README, nice touch :)

Automatic review by ventral.org:

http://ventral.org/pareview/httpgitdrupalorgsandboxnickschuch1726760git

Minor issues, but nice to fix.

Manual review:

• I would put in another name in the .info file. "Provides variables for SASS module." isn't really a good name, and if I'm not mistaken the name should reflect the machine-name of your module. The description is perfect for describing what you module does.
• Consider adding a 'Configure' link in your .info file so it'll show up in the Modules-list. See http://www.drupalcoder.com/blog/configure-links-on-modules-overview-page-in-drupal-7
• You could also include a checkbox in the "SASS Mixin Variables" admin panel for wether or not to automatically run "Clear Cache" after you save and rebuild mixin. Saves the developer a few steps.

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.

Accidentally added a title.