2
\$\begingroup\$

When you click on a span it enters the value into the input. That works fine, but as you can see the JS code is very big and there must be a better way to do this, but like I said I'm still learning. Maybe someone can give me a pointer in the right direction.

I suppose I have to use a variable somewhere but can't really figure it out on my own.

jsFiddle

jQuery(document).ready(function() {
 $("#spanval1").click(function(){
 $('#hourvalue').val($('#spanval1').text());
 });
 $("#spanval2").click(function(){
 $('#hourvalue').val($('#spanval2').text());
 });
 $("#spanval3").click(function(){
 $('#hourvalue').val($('#spanval3').text());
 });
 jQuery('.hour_dropdown').hide()
 jQuery("#more").click(function() {
 $('.hour_dropdown').fadeToggle(200);
 });
});​
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Aug 6, 2012 at 13:35
\$\endgroup\$
0

3 Answers 3

4
\$\begingroup\$

Building off of Danny's answer, if you have access to the HTML, you can add a class to each of your span tags and bind an event to that class:

<span id="spanval1" class="spanval">1</span>
<span id="spanval2" class="spanval">2</span>
<span id="spanval3" class="spanval">3</span>

The event call can become more generic by using the event's local context 'this'. From the .bind() documentation:

Within the handler, the keyword this refers to the DOM element to which the handler is bound. To make use of the element in jQuery, it can be passed to the normal $() function.

'this' inside of the click event will be the DOM element that was clicked on that caused the event to fire. Then you would need only the following to setup the click event for all of the elements with the class 'spanval':

$(".spanval").click(function(){
 $('#hourvalue').val($(this).text());
});

And with the rest of the script:

$(document).ready(function() {
 $(".spanval").click(function(){
 $('#hourvalue').val($(this).text());
 });
 $('.hour_dropdown').hide();
 $("#more").click(function() {
 $('.hour_dropdown').fadeToggle(200);
 });
});​
answered Aug 6, 2012 at 19:31
\$\endgroup\$
2
  • \$\begingroup\$ Typo alert: The first selector there should end with the letter L. \$\endgroup\$ Commented Aug 6, 2012 at 21:19
  • \$\begingroup\$ Thanks. But can you perhaps explain me how (this) knows the value of the span? \$\endgroup\$ Commented Aug 6, 2012 at 22:20
2
\$\begingroup\$

First off, this refers to the element you clicked on, so we can simplify your code a bit my replacing your second #spanval like so:

$("#spanval1").click(function(){
 $('#hourvalue').val($(this).text());
});

Next, you can extract that click handler into it's own function, allowing the index to be passed in as a parameter.

function handleClick(num){
 $("#spanval"+num).click(function(){
 $('#hourvalue').val($(this).text());
 });
}

This will allow us to replacing all of those click handlers in the main function with this:

handleClick(1);
handleClick(2);
handleClick(3);

Now, we're still being repetitive, so lets reduce the repetition using a for loop:

jQuery(document).ready(function() {
 var maximumNumber = 3;
 for(var n = 1; n <= maximumNumber; n++){
 handleClick(n);
 }
 jQuery('.hour_dropdown').hide()
 jQuery("#more").click(function() {
 $('.hour_dropdown').fadeToggle(200);
 });
});​
answered Aug 6, 2012 at 13:55
\$\endgroup\$
0
\$\begingroup\$

Here are some points worth considering:

  1. Cache your selectors! If there's one thing you can do to increase performance, it's caching selectors. Traversing the DOM is an expensive operation, and should be done only when absolutely necessary.
  2. Use an attribute selector (span[id^=spanval]). This'll allow you to select all spans who's ID starts with spanval (if not all of them are spans, you can use the attribute selector on its own ([id^=spanval]) which is a tad slower but works just as well).

  3. Whenever possible, try not to use the jQuery constructor. It's much faster to call the static methods from the jQuery namespace (when they're available). So instead of $(this).text(), try using $.text(this).


With that in mind, here's some sample code:

jQuery(document).ready(function($) {
 var $hourVal = $('#hourvalue'),
 $hourDropdown = $('.hour_dropdown');
 $('span[id^=spanval]').click(function(){
 $hourVal.val( $.text(this) );
 });
 $hourDropdown.hide()
 $("#more").click(function() {
 $hourDropdown.fadeToggle(200);
 });
});​
answered Aug 6, 2012 at 21:16
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.