Return to Answer

Personally I would probably do it this way. It's a bit of a different approach; you don't need this, you don't need new, and it's less code:

function ColorBox(target_id, btn) { var $parent = $("#" + target_id).parent(); var $color = $('<div class="color">ops</div>').appendTo($parent); var $button = $('<button class="change" value="' + btn + '">' + btn + '</button>').appendTo($color); $button.on( 'click', function (event) { $parent.css('background', $button.val()); }); } $(document).ready(function () { ColorBox('target', 'red'); }); 

Whether you take this approach or do something more like @Joe's answer, there is one thing you should definitely change to work like I have it in this code. Your parent and color variables are both already jQuery objects; there is no need to wrap them in additional $() calls when you use them. So change the names of these variables to include the $ prefix as a reminder that they are jQuery objects, and then just use them directly where you need them instead of the extra $() wrapper.

If you use self as in @Joe's answer, then it would be code like:

self.$parent = $("#" + target_id).parent(); self.$color = self.$parent.append(...); 

The $ prefix on these names isn't necessary, but it's a common convention to indicate a variable or property that is a jQuery object. It helps keep straight whether you need to use another $() around it.

Finally, are you aware that your parent and color variables are the same element? It looks like you're expecting color to be the <color> element, but it isn't. I changed the code so it is that element.

Personally I would probably do it this way. It's a bit of a different approach; you don't need this, you don't need new, and it's less code:

function ColorBox(target_id, btn) { var $parent = $("#" + target_id).parent(); var $color = $('<div class="color">ops</div>').appendTo($parent); var $button = $('<button class="change" value="' + btn + '">' + btn + '</button>').appendTo($color); $button.on( 'click', function (event) { $parent.css('background', $button.val()); }); } $(document).ready(function () { ColorBox('target', 'red'); }); 

Whether you take this approach or do something more like @Joe's answer, there is one thing you should definitely change to work like I have it in this code. Your parent and color variables are both already jQuery objects; there is no need to wrap them in additional $() calls when you use them. So change the names of these variables to include the $ prefix as a reminder that they are jQuery objects, and then just use them directly where you need them instead of the extra $() wrapper.

If you use self as in @Joe's answer, then it would be code like:

self.$parent = $("#" + target_id).parent(); self.$color = self.$parent.append(...); 

The $ prefix on these names isn't necessary, but it's a common convention to indicate a variable or property that is a jQuery object. It helps keep straight whether you need to use another $() around it.

Also, be aware that your parent and color variables are the same element. It looks like you're expecting color to be the <color> element, but it isn't. I changed the code so it is the <color> element.

Personally I would probably do it this way. It's a bit of a different approach; you don't need this, you don't need new, and it's less code:

function ColorBox(target_id, btn) {   var $parent = $("#" + target_id).parent(); var $color = $parent.append('<div class="color" >ops</div>'); $color.append('<button class="change" value="' + btn + '">' + btn + '</button>'); $parent.find(".change").on( 'click', function (event) { $parent.css('background', $(event.target).val()); }); } $(document).ready(function () { ColorBox('target', 'red'); }); 

Whether you take this approach or do something more like @Joe's answer, there is one thing you should definitely change to work like I have it in this code. Your parent and color variables are both already jQuery objects; there is no need to wrap them in additional $() calls when you use them. So change the names of these variables to include the $ prefix as a reminder that they are jQuery objects, and then just use them directly where you need them instead of the extra $() wrapper.

If you use self as in @Joe's answer, then it would be code like:

self.$parent = $("#" + target_id).parent(); self.$color = self.$parent.append(...); 

The $ prefix on these names isn't necessary, but it's a common convention to indicate a variable or property that is a jQuery object. It helps keep straight whether you need to use another $() around it.

Also note this simplification:

$("#" + self.parent.attr('id') + " .change")... 

can be written much more simply and clearly as:

$parent.find(".change")... 

or:

self.$parent.find(".change")... 

Personally I would probably do it this way. It's a bit of a different approach; you don't need this, you don't need new, and it's less code:

function ColorBox(target_id, btn) { var $parent = $("#" + target_id).parent(); var $color = $('<div class="color">ops</div>').appendTo($parent); var $button = $('<button class="change" value="' + btn + '">' +  btn + '</button>').appendTo($color); $button.on( 'click', function (event) { $parent.css('background', $button.val()); }); } $(document).ready(function () { ColorBox('target', 'red'); }); 

Whether you take this approach or do something more like @Joe's answer, there is one thing you should definitely change to work like I have it in this code. Your parent and color variables are both already jQuery objects; there is no need to wrap them in additional $() calls when you use them. So change the names of these variables to include the $ prefix as a reminder that they are jQuery objects, and then just use them directly where you need them instead of the extra $() wrapper.

If you use self as in @Joe's answer, then it would be code like:

self.$parent = $("#" + target_id).parent(); self.$color = self.$parent.append(...); 

The $ prefix on these names isn't necessary, but it's a common convention to indicate a variable or property that is a jQuery object. It helps keep straight whether you need to use another $() around it.

Finally, are you aware that your parent and color variables are the same element? It looks like you're expecting color to be the <color> element, but it isn't. I changed the code so it is that element.

Personally I would probably do it this way. It's a bit of a different approach; you don't need to use this at all:

function ColorBox(target_id, btn) { var $parent = $("#" + target_id).parent(); var $color = $parent.append('<div class="color" >ops</div>'); $color.append('<button class="change" value="' + btn + '">' + btn + '</button>'); function changeColor(element) { $parent.css('background', $(element).val());  return true; } $("#" + $parent.attr('id') + " .change").on( 'click', function (event) { changeColor(event.target); });   } $(document).ready(function () { ColorBox('target', 'red'); }); 

Whether you take this approach or do something more like @Joe's answer, there is one thing you should definitely change to work like I have it in this code. Your parent and color variables are both already jQuery objects; there is no need to wrap them in additional $() calls when you use them. So change the names of these variables to include the $ prefix as a reminder that they are jQuery objects, and then just use them directly where you need them instead of the extra $() wrapper.

If you use self as in @Joe's answer, then it would be code like:

self.$parent = $("#" + target_id).parent(); self.$color = self.$parent.append(...); 

The $ prefix on these names isn't necessary, but it's a common convention to indicate a variable or property that is a jQuery object. It helps keep straight whether you need to use another $() around it.

Personally I would probably do it this way. It's a bit of a different approach; you don't need this, you don't need new, and it's less code:

function ColorBox(target_id, btn) { var $parent = $("#" + target_id).parent(); var $color = $parent.append('<div class="color" >ops</div>'); $color.append('<button class="change" value="' + btn + '">' + btn + '</button>'); $parent.find(".change").on( 'click', function (event) { $parent.css('background', $(event.target).val()); }); } $(document).ready(function () { ColorBox('target', 'red'); }); 

Whether you take this approach or do something more like @Joe's answer, there is one thing you should definitely change to work like I have it in this code. Your parent and color variables are both already jQuery objects; there is no need to wrap them in additional $() calls when you use them. So change the names of these variables to include the $ prefix as a reminder that they are jQuery objects, and then just use them directly where you need them instead of the extra $() wrapper.

If you use self as in @Joe's answer, then it would be code like:

self.$parent = $("#" + target_id).parent(); self.$color = self.$parent.append(...); 

The $ prefix on these names isn't necessary, but it's a common convention to indicate a variable or property that is a jQuery object. It helps keep straight whether you need to use another $() around it.

Also note this simplification:

$("#" + self.parent.attr('id') + " .change")... 

can be written much more simply and clearly as:

$parent.find(".change")... 

or:

self.$parent.find(".change")...