[D7] JS Spellcheck

Hi,

there is a little bit work required. Please fix errors and warnings listed here: http://ventral.org/pareview/httpgitdrupalorgsandboxkbasarab1762300git

(please check the Drupal coding standards)

Hello,

i mean in your javascript in line 13, you need to remove the ','. In Javascript it's forbidden to make a ',' after the last array item.

Sorry for my bad english, I'm not a native speaker.

Changing to needs work

Hi kbasarab,

Thanks very much for contributing this very nice module.

On top of previous comments, I would like to add a rather critical issue that would be really easy to fix:

1 - In file js_spellcheck.admin.inc, at line 40:

function js_spellcheck_get_dictionaries() {
  $path = libraries_get_path('js_spellcheck');
  [...]

There is a call to libraries_get_path, which is great for integration with the Libraries API, but this would require to add a dependency from your module to the libraries one.
So I would simply recommend that you add one line of code in your file js_spellcheck.info:
Line 2, for example, add the following code:

dependencies[] = libraries

Currently this results in the administration config page to simply crash:
Page: admin/config/js_spellcheck
> Fatal error: Call to undefined function libraries_get_path()

2 - In file js_spellcheck.module, at line 31:

    'description' => 'Configure which content types to enable spellchecker on',

Perhaps you should consider adding a call to t() function, as you did in all other places in your module where you have a 'description' property.
I think that's the only place I found in your module where a call to t could be missing.

So far, that's all I could find, but I think we should be giving your module another round of reviews after all comments reported above have been fixed.

Your module looks good and has a pretty cool functionality. Well done! and I hope these comments will help you to improve it.

Cheers!

js_spellcheck.module

line #31 | Do not use t() in hook_menu()

Hi kbasarab,

Following up with monymirza's latest reply, I wanted to personally apologize for inducing you into making this unnecessary change, suggested at #5:

2 - In file js_spellcheck.module, at line 31:

    'description' => 'Configure which content types to enable spellchecker on',

Perhaps you should consider adding a call to t() function, as you did in all other places in your module where you have a 'description' property.
I think that's the only place I found in your module where a call to t could be missing.

I didn't pay enough attention this line was in a hook_menu implementation and it was a mistake I made.
Luckily, this time, it won't result in too much waste of your time, since this seems to be a rather small change.

I will surely watch out for that next time.
Sorry again for the inconvenience.
Cheers!

@monymirza: Thanks for catching this up and letting us know.

Hi,

Please read review bonus and get 'PAReview: review bonus' to get your project reviewed.

Hi ,

In the file mobile_menu_toggle.module

There is 'file doc comment' is missing at the line 2 and function doc comment is missing at the lines 3,11 and 21

Hi srutheesh,

Could you please provide specific links/URLs to the GIT code you are mentioning?
I have checked at: http://drupalcode.org/sandbox/kbasarab/1762300.git/tree/17a4c1a
which seems to be the latest version (See all commits and 17a4c1a) of the project's code tree and was unable to find the code you mentioned in #11 and #12.
I am getting confused here and wondering if you are not posting these comments in the wrong project.

Thanks in advance for your answer.

Hello kbasarab,

Thanks for greate module!

Plese add .install file, and using hook_uninstall() delete variables on module uninstall.

Cheers!

How could I have missed that while checking js_spellcheck.admin.inc ....
Really good catch vladimir-m!

Thanks a lot for spotting that out.

Ventral automated code review: (http://ventral.org/pareview/httpgitdrupalorgsandboxkbasarab1791874git)

FILE: /var/www/drupal-7-pareview/pareview_temp/mobile_menu_toggle.module
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
3 | ERROR | Missing function doc comment
11 | ERROR | Missing function doc comment
16 | ERROR | break statement indented incorrectly; expected 6 spaces, found 4
21 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mobile_menu_toggle.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
6 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

All minor stuff really. I'm sure you'll sort that quickly. With my own personal submission I've just gotten in the habit of always running it before I go back to "needs review"

Actual testing in Drupal Environment:

I followed your readme. Pretty good and thorough details. Good job. Got it all there, set to content type.. .went to misspell "banananas" and... nada? Okay, so I open console, reload and get:

TypeError: Drupal.behanviors is undefined
attach: function (context, settings) {
js_spe...?mgwjml (line 3)

So you've got a typo in your code :)

So I went to remove the typo for you... and run the check. I added the word "bananannnaas" which I'm pretty sure isn't spelled right! Hit save... and it said spellchecker finished with no errors. Hmmm? There must be a bigger issue at hand here.

I tried preview, with the word "dynosaurs" in there as well. It appears you don't do anything on preview either? I imagine you'll want to hook into preview with this as well maybe? Just a thought. Hit save, again, spellchecker finished with no errors.

I'm going to wrap up my code review here since it doesn't work and your code may wind up changing.

As an additional bug, I've been testing some other modules since enabling this, and when dealing with multiple images this spellcheck seems to pop up an additional time for every image attached to the node... something is most definitely fishy!

Just pulled the latest version, uploaded three images, put in four correctly spelled words, and the JS spellcheck kept popping up endlessly. Firefox finally asked if I wanted to prevent any more popups.

I'll review the code for you once you get these bugs sorted out.

I've also got an empty file field for uploading PDF's with the content type as well.

I also want to add... the code was prompting me to purchase javasript spellcheck, at $99 a whack per site...

Not sure if that's expected or not.

I think this is a great module, I only have a question/suggestion about the intended functionality. I created a bunch of fields, another long text and summary, text field, text area and typed some correct words and typos into each. All but the text box's were highlighted red under the mis-spellings, but when I went to save the little pop-up widget only went through words that were in the original body text field, not any of the other fields. The other functionality, like selecting a new word from a right-click is there, but not the on-submit action. I think it ought to check all the fields and/or have the option to select which fields are spellcheck'd.

Your field_skip_spellcheck technique is interesting, but ultimately kinda weird - we have enough magical names for things in Drupal, don't add more if you can help it :)

But that's fairly minor, looks good to me.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Well it's been over a month and the code still looks pretty good.

Thanks for your contribution, kbasarab!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Congratulations and thanks very much for your contributions and great work kbasarab!

It seems it took quite a while, with many updates and I assume quite some work, but finally this great module can move out of sandbox.

I will certainly keep following the module with a great interest in its project tracker and hopefully will try to come up with useful/constructive issues or feature requests.

Thanks again very much for everyone for the great reviews, testing and contributions.
Cheers!