Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
28 Jul 2011 at 06:16 UTC
Updated:
31 Aug 2018 at 10:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
bboldi commentedI have fixed the branch naming, and created a 6.x version.
Comment #2
pillarsdotnet commentedWill test shortly.
Comment #3
bboldi commentedThanks!
Comment #4
tim.plunkettPlease mark one of your two applications as "closed (duplicate)": #1229776: [D7] b2 Drupal Plus or #1232020: [D7] b2 nice comments.
You only need to be reviewed and approved once.
Thanks!
Comment #5
tim.plunkettI started to review the code and make coding standards fixes, and after a while I realized I'd rewritten your whole module. Sorry for being so presumptuous, I had the best intentions.
There is a new naming convention for CSS files, the goal is to split up the functional from the aesthetic. I've also moved the display: none; from the module to the CSS for when JS is disabled.
Not many people have found
hook_form_BASE_FORM_ID_alter(), as it is a new Drupal 7 function, but it allows you to find all comment forms without the use of strpos.#attached allows you to add CSS and JS to a form directly, allowing others to conditionally unset it in another form_alter.
The magic ___ form_ids that you're using are largely unnecessary, as the jQuery can just find the sibling of whichever is clicked.
I've also converted the JS to use Drupal.behaviors, you can read more about the benefits of this here: http://drupal.org/node/171213#behaviors
The focus was being set twice, so I removed the extra one. I believe comment body is the normal default.
I'm not sure exactly what the conditional is at the bottom of the JS, but you should document it.
Please review these changes, and if you don't disagree with them, apply them. Then set back to "needs review" and you'll be pretty much ready to go.
A final note, I might consider removing b2 from the namespace, and just going with nice_comments. Putting your company's namespace in a module name is discouraged, and you are free to put that on the project page or in a README.txt. I know I generally shy away from modules named like this. However, this is my personal opinion, and will not affect your review.
Comment #6
bboldi commentedFirst of all: Thanks for the review, and thanks for taking the time to doing an in depth analysis of the module! Much appreciated, I have learned a lot! I will modify the module taking your suggestions into consideration, and submit it for a re-review when it's ready.
Comment #7
bboldi commentedI have fixed the coding standard problems found by tim.plunkett . Please re-review the code.
Thanks!
Comment #8
tim.plunkettThis is a really simple, but high quality module. Despite a little assist from me, you demonstrated your knowledge of the Drupal API, and I have given you full project creation rights. Use this ability carefully!
Thanks for your patience.
Comment #9
bboldi commentedThank You!
Comment #10
tim.plunkettJust to let you know, I just used your module as a demo at the Philadelphia Drupal meetup, as a demo of how to incorporate custom JS into a module. You're famous! :)
Comment #11
bboldi commentedTim, You made my day :) Thanks!
Comment #13
avpaderno