Posted by forngren on September 11, 2006 at 9:58am
Jump to:
| Project: | ChipIn module |
| Version: | 4.7.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Issue Summary
As promised, here's my review of your module;
Note that I haven't tested the module, this is just a review of the source.
images/
Do the logo really have to be .gif? in most cases PNG is preffered.
js/chipin_textarea.js
Looks good, coding standars could be improved.
README.txt
Nice :D
chipin.css
CSS hacks is ok, but it does contain some typos; http://jigsaw.w3.org/css-validator/
chipin.module
Looks ok, t() use a bit fuzzy. Note: I'm having big trouble to use t() correctly.
<?php
$form['info'] = array(
'#type' => 'markup',
'#value' => '<p>'
. t('ChipIn empowers individuals to connect with people in their social network to collect money for a personal cause, to purchase a gift, or for community fundraising. ChipIn makes this <strong>social ecommerce</strong><sup>SM</sup> process of connecting and collecting fun, easy and secure. ChipIn believes that positive change will result from enabling groups to harness the power of giving. That\'s the ChipIn mission.')
. ' ' . t('Learn more on the %chipin-link.', array('%chipin-link' => l(t('ChipIn help page'), 'admin/help/chipin')))
. '</p>',
);
// Could perhaps be merged to?
$form['info'] = array(
'#type' => 'markup',
'#value' => '<p>'
. t('ChipIn empowers individuals to connect with people in their social network to collect money for a personal cause, to purchase a gift, or for community fundraising. ChipIn makes this <strong>social ecommerce</strong><sup>SM</sup> process of connecting and collecting fun, easy and secure. ChipIn believes that positive change will result from enabling groups to harness the power of giving. That\'s the ChipIn mission. Learn more on the %chipin-link.', array('%chipin-link' => l(t('ChipIn help page'), 'admin/help/chipin')))
. '</p>',
);
?><?php
if ($access) {
$options[] = t('Show if the following PHP code returns <code>TRUE</code> (PHP-mode, experts only).');
$description .= t(' If the PHP-mode is chosen, enter PHP code between %php. Note that executing incorrect PHP-code can break your Drupal site.', array('%php' => theme('placeholder', '<?php ? >')));
}
// Unecceary whitespace for translators?
if ($access) {
$options[] = t('Show if the following PHP code returns <code>TRUE</code> (PHP-mode, experts only).');
$description .= ' '. t('If the PHP-mode is chosen, enter PHP code between %php. Note that executing incorrect PHP-code can break your Drupal site.', array('%php' => theme('placeholder', '<?php ? >;')));
}
?>Again, I suck at t() use, most of my patches fail cause of it.
Overall it's a nice module and the coding standards is optional in contrib modules.
Cheers Johan Forngren
Comments
#1
Those super long strings should probably be split into multiple lines too - just look at how long the scrollbar on this page is.
#2