Alright so a few things, and this is mainly to help with good practices rather than merely solve the answer(as @BhojendraNepal has done well). Lets start with the final code:
var $tagsInput = $('input').filter('[name="tags"]');
$('.tagSelect').on({
click:function(){
var data = $(this).attr('data'),
tags;
if($tagsInput.val()){
tags = $tagsInput.val() + ',';
} else {
tags = '';
}
$tagsInput.trigger('tagChange',[tags,data]);
}
},'a');
$tagsInput.on({
tagChange:function(e,tags,data){
$tagsInput.val(tags + data);
}
});
A couple things to note:
- The item you reference multiple times is cached
- The
.on('click') uses the closest parent as delegation driver, rather than the document
- The change of the
input value has been modified to take place in its own event structure, rather than within the click event
Why would you do all that?
If you reference something multiple times, you should cache it because jQuery would otherwise need to re-query the DOM. Querying the DOM is the most taxing part of JavaScript, so minimizing this is the key to performant JavaScript.
Using event delegation helps out for a number of reasons, however your use of delegating from the top-level document is the least performant way to do this (even the jQuery documentation says to avoid this in their .on() documentation under event performance). The ideal scenario is to use the closest-scoped parent as possible, and luckily you have identified one in the .tagSelect class. Using that as your delegation driver will provide the same benefits, but even better performance.
The segregation of the value change is a bit more abstract, but useful to understand if you ever plan this application to get larger. Perhaps you will want something else to cause the value change in the future, or perhaps your click functionality will grow to be massive, who knows. By separating the click functionality (setting the value for tags) from how it affects the input (changing the value), you are creating a more extensible code methodology that is separated by concerns. I'm sure I'll get some people saying "it doesn't matter for an application of this size", but its a good methodology to get familiar with because it can save many headaches in the future.
Just my two cents. As mentioned, @BhojendraNepal's answer works just fine because your issue was using .html() instead of .val(), just providing some extra info.
val()instead of.html, because.htmlreturns inner HTML but input does not have inner html ...