Page 1 of 1

JS/jQuery critique

Posted: Tue Jun 25, 2013 1:50 pm
by thinsoldier
Hello I just a built a small tool for myself but I really think the JS/jQuery could be a lot better.
A lot more object oriented I guess.
The script is toward the bottom of the file.

It simply observes a list of radio buttons and a text field. If a radio is chosen and the field contains all integers it makes an ajax request and updates the contents of a div.

Then further down it watches a select field and updates another select field.

Any suggestions?

Code: Select all

<!DOCTYPE html>
<html>
<head>
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.1/jquery.min.js"></script>
</head>
<body>
<form>
<h2>From</h2>
<div id="fromlist">
<ul>
	<li> 
	<input type="radio" id="from0" value="download" name="from_table">
	<label for="from0">download</label> </li>
	<li> 
	<input type="radio" id="from1" value="competition_policy" name="from_table">
	<label for="from1">competition_policy</label> </li>
	<li> 
	<input type="radio" id="from2" value="licencing" name="from_table">
	<label for="from2">licencing</label> </li>
	<li> 
	<input type="radio" id="from3" value="news" name="from_table">
	<label for="from3">news</label> </li>
	<li> 
	<input type="radio" id="from4" value="decisions" name="from_table">
	<label for="from4">decisions</label> </li>
</ul>
</div>

<div>
id# <input type="text" name="from_id" id="from_id_field">
</div>


<div id="preview">

</div>


<h2>To</h2>

<div>
Table 
<select id="tolist" name="to_table">
	<option value="">Select</option>
	<option value="download">download</option>
	<option value="competition_policy">competition_policy</option>
	<option value="licencing">licencing</option>
	<option value="news">news</option>
	<option value="decisions">decisions</option>
</select>
</div>


<div>
Category <select name="category[]" id="cat"></select>
</div>

<br>
<input type="submit">

</form>



<script>
$("#fromlist").click(watchFromList);
$("#from_id_field").keyup(watchFromID);
$("#tolist").change(watchToList);

var from_table;
var from_id;
var to_table;

function watchToList(event)
{
	to_table = event.target.value;
	// returns long string of option tags <option value="34">foo</option>
	$.get('_section-api.php',{'cmd':'categories','table':to_table })
		.done( function(data) { $('#cat').html(data); });
}


function watchFromList(event)
{
	if(event.target.value)
	{
		from_table = event.target.value;
		ajaxForFrom();
	}
}

function watchFromID(event)
{
	var test = event.target.value.match(/^\d{1,}$/)
	if(test){ 

		from_id = event.target.value;
		ajaxForFrom();
	} else {
		console.log('id test fail');
		$('#preview').html('');
	}
}



function ajaxForFrom()
{
	if( from_table && from_id )
	{
		// sample response: {"id":"196","category":"Revoked","headline":"Holdings Revocation Notice"}
		$.getJSON('_section-api.php',{'cmd':'preview','table':from_table, 'id':from_id })
			.done(function(data) { updatePreview(data) })
	}
	else {
		console.log('not enough info to pull FROM item details')
	}
}

function updatePreview( jsn )
{
	$('#preview').html('');
	var string = '';
	for( i in jsn )
	{
		 string += i + ': ' + jsn[i] + "<br>";
	}
	$('#preview').html(string);
}

</script>

</body>
</html>

Re: JS/jQuery critique

Posted: Tue Jun 25, 2013 5:51 pm
by pickle
There's nothing really wrong with the way you're doing it as far as I can see. Generally though, accepted practice is to use the .on() handler. And since your functions are so short, I'd simply include them as an anonymous function in your call to .on()...

Code: Select all

$("#fromlist").on('click',function(e){
   //code you currently have in watchToList()
  //"this" refers to "event.target" - could simplify the code a bit
});