HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

jQuery list widget

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
listjquerywidget

Problem

I have been writing a jQuery widget for a couple of days now. Originally I needed this functionality for something I am writing at work, but I felt like I could abstract it a little more so here I am. I think it's about finished for now, I may add some features depending on how It gets used (if it does).

This is my first jQuery widget and I have been programming in JavaScript for just over a month now, so I don't think the code will be particularly great, but I've tried my hardest.

I have done some vague testing and it does seem to work in all browsers.

My main gripes are validation of the options, of which I don't really know how to do, however, I am going to do some proper research on this and come back to it.

There a few other problems with it but I think that this is not the right place to ask about those, so will leave it for now.

I would just like some feedback on my coding, how to improve/do it better, now and in the future. It's quite a big chunk of code but I didn't see anything in the FAQ about length of code.

I've also attached a jsFiddle so you can mess with it: http://jsfiddle.net/jCVzp/2/

I've tried to comment as best as I can and have noted all the options in the file description, and more guidance needed, just ask.

All the CSS is present in the jsFiddle, didn't think it was necessary to post it here.

Any time and help is greatly appreciated.

```
(function($) {
/*
* Document : list-widget-0.1
* Created on : 06-Aug-2012, 20:53:49
* Author : aydin hassan
* Description:
* This widget is designed to be attatched to a div
*
* Using the options you can specify one of three list types:
* 1.) a normal list if items (normal)
* 2.) a list of items with a cross/tick img depending on its status (act-deact)
* ---Clicking the image will toggle its status (cha

Solution

From a once over:

-
Do not do this:

if(options.selectable != null)


do either

if(options.selectable)


or

if(options.selectable !== null) //Notice the ==


  • You are not consistent with semicolons, use jshint.com to fix your code



  • Extract some constants out, particularly widths and heights in your element building code



  • You use both single and double quotes to delimit strings, pick one, preferably single quotes



  • Considering using $().toggleClass(), you seem to be toggling a few times classnames yourself



  • I would allow toggleCallBack to cancel the toggeling so that I could use it validate the toggle



Other than that I think your code is solid.

Code Snippets

if(options.selectable != null)
if(options.selectable)
if(options.selectable !== null) //Notice the ==

Context

StackExchange Code Review Q#14525, answer score: 2

Revisions (0)

No revisions yet.