views:

209

answers:

3

I've created a JavaScript object to hold onto a value set by a user checking a checbox in a ColorBox.

I am relatively new to jQuery and programming JavaScript "the right way" and wanted to be sure that the below mechanism for capturing the users check action was a best practice for JavaScript in general. Further, since I am employing jQuery is there a simpler method to hold onto their action that I should be utilizing?

function Check() {
    this.Checked = false;
}

obj = new Check;

$(document).ready(function() {
    $('.cboxelement').colorbox({ html: '<input id="inactivate" type="checkbox" name="inactivatemachine">&nbsp;<label for="inactivate">Inactivate Machine</label>' });

    $(document).bind('cbox_cleanup', function() {
        obj.Checked = $.fn.colorbox.getContent().children('#inactivate').is(':checked');
    });

    $(document).bind('cbox_closed', function() {
        if ($($.fn.colorbox.element()).attr('id').match('Remove') && obj.Checked) {
            var row = $($.fn.colorbox.element()).parents('tr');
            row.fadeOut(1000, function() {
                row.remove();
            });
        }
    });
});
A: 
  • $($.fn.colorbox.element()) is redundant. $.fn.colorbox.element() is already a jquery element.

  • It's common use (in the examples i watched, at least) to prepend a $ to variables referencing jquery elements.
    So, var $rows = $.fn.colorbox.element().parents('tr'); gives instantly the idea that it is referencing jquery element(s).

  • I am afraid fadeOut won't work on rows in IE6 (if i recall correctly). You should be able to hide all the content inside the <tr> before removing it.

Can't help on the "simplify" thing because i don't know the colorbox's best uses.

Alex Bagnolini
Unfortunately, $.fn.colorbox.element() doesn't return a jQuery element but will in the next release. I'll check out the fadeout stuff in IE6, thanks for the heads up. My largest concern is the use of my custom Check() object for the purpose of capturing the user's action.
ahsteele
Technically, that would be "prepend a `$`"
Justin Love
Corrected, thank you
Alex Bagnolini
+1  A: 

You could capture the reference in a closure, which avoids global data and makes it easier to have multiple Checks. However, in this case it appears to be binding to the single colorbox, so I don't know that you could usefully have multiple instances.

function Check() {
    this.Checked = false;

    var obj = this; // 'this' doesn't get preserved in closures

    $(document).ready(function() {
       ... as before
    )};
}

var check = new Check; // Still need to store a reference somewhere.
Justin Love
ColorBox dynamically renders its content so there is only on per page. I need to read up more on closures but this seems much cleaner than having a *global* variable running around.
ahsteele
+2  A: 

Personally, I would attach the value(s) to an object directly using jQuery's built-in data() method. I'm not really entirely sure what you are trying to do but, you can, for instance, attach values to a "namespace" in the DOM for use later one.

$('body').data('colorbox.checked',true);

Then you would retrieve the value later by:

var isChecked = $('body').data('colorbox.checked');

You run the data() method on any jquery object. I would say this is best-practice as far as jQuery goes.

KyleFarris
I didn't know about jQuery's `data()` method. Given the nature of ColorBox that seems the best fit for my particular use. Is there any guidance when attaching values to a "namespace"?
ahsteele
@ahsteele: Well, the "namespace" is really string based (hence the quotes around the word in my answer). Basically you are adding a a key->value pair to an object. Something like (for demo purposes only): `document.body.data['colorbox.checked'] = true`. This is so that some other plugin doesn't accidentally overwrite your key->value pair (which is quite possible with a generic variable like 'checked'). You could just as legitimately do: `$('body').data('colorbox_checked',true)`. instead of the dot-notation. Like I said it's "namespaced". Good luck!
KyleFarris