Creating <select> element and append data to it

Question

I've made the following and have a few questions:

1. Do I have some (big) mistakes in this approach? (bad practices, 'this code is trash'...)

   1.1 If I do, can you suggest what to fix?

2. Are appended element created in good way?
3. Code in success function seems to be a little long. Any way to refactor?

enter image description here

CSS code

.no-display-table {
 display: none;
}

HTML code

<div class="row">
 ...
 <!-- empty div will hold <select> which will be crated on ajax success --> 
 <div class="append-select col-md-3"></div>
 ...
</div>
...
<!-- onLoad => 'display: none', <tr> will be dynamically crated on Ajax Success -->
<div class="row">
 <div class="col-md-4 col-md-offset-2">
 <table id="options-table" class="table table-striped">
 <thead> <tr><th> Selected options </th></tr> </thead>
 <tbody></tbody>
 </table>
 </div>
</div>

JavaScript code

$(function() {
 $('#options-table').addClass('no-display-table');
 $('#product-option-select').change(function() {
 if($('.append-select select').length > 0) 
 $('.append-select select').remove();
 var selectedOptionId = $(this).val();
 $.ajax({
 type: 'get',
 url: '../option_types/' + selectedOptionId,
 success: function(data) {
 var element = '<select id="value-select" class="form-control">';
 element += '<option selected disabled>Select value...</option>';
 for(var i=0; i<data.length; i++) {
 element += '<option value="' + data[i].id + '">' + data[i].name + '</option>';
 }
 element += '</select>';
 $('.append-select').append(element);
 $('.append-select').on('change', '#value-select', function() {
 $('#options-table').removeClass('no-display-table');
 var selectedValue = $('#value-select option:selected').text();
 var element = '<tr><td>' + selectedValue + '</td>';
 element += '<td><a class="remove"> x </a></tr>';
 $('#options-table tbody').append(element);
 $('#options-table').on('mouseover', '.remove', function() {
 $('.remove').css('cursor', 'pointer');
 });
 $('#options-table').on('click', '.remove', function() {
 $(this).parent().parent().remove();
 if($('#options-table tbody tr').length == 0)
 $('#options-table').addClass('no-display-table');
 });
 });
 }
 });
 });
});

Answer 1

My 2 cents:

• You could use $().hide() instead of assigning your custom hiding css class

• Similarly you could use $().show() instead of removing your custom hiding css class

• You do not have to check the length of a jQuery query, just call remove(), it will work.

• Your building of html inside the success callback is ugly, have a helper function to construct it and consider using DOM functions instead of HTML building. I personally tend to show/hide the select element and then rebuild the list with this helper function:

function setOptions( id , array )
{
 //Get the element
 var select = document.getElementById( id ) , i 
 //Remove potentially existing children, somewhat heartless
 while (select.firstChild)
 select.removeChild(select.firstChild);
 //Add the new children
 for( i = 0 ; i < array.length ; i++ )
 {
 var element = new Option( array[i].id , array[i].name );
 select.appendChild(element); 
 } 
}

• Assiging the listener in the success callback is also too much, consider building a listener that takes into account new elements on document instead $(document').on('change', etc.

• Same for the mouseover / click event listeners

Simply having the document element ( or any other parent element that stays in the DOM ) and the selector inside the on call should activate the listeners for new elements.

function initializeListeners()
{
 $(document).on('change', '#value-select', function() {
 $('#options-table').removeClass('no-display-table');
 var selectedValue = $('#value-select option:selected').text();
 var element = '<tr><td>' + selectedValue + '</td>';
 element += '<td><a class="remove"> x </a></tr>';
 $('#options-table tbody').append(element);
 $('#options-table').on('mouseover', '.remove', function() {
 $('.remove').css('cursor', 'pointer');
 });
 $('#options-table').on('click', '.remove', function() {
 $(this).parent().parent().remove();
 if($('#options-table tbody tr').length == 0)
 $('#options-table').addClass('no-display-table');
 });
 });
}

• Srle: Assiging the listener in the success callback is also too much....Same for the mouseover / click event listeners Can you please provide code for that?