patternjavascriptMinor
Toggle functions for drop down for a clock time widget
Viewed 0 times
timedropdownforclockfunctionswidgettoggle
Problem
I'm fairly new to JQuery/JavaScript and I'm trying to refactor the following code:
I haven't had much luck figuring out how to improve the code. Here is a CodePen of the current working code and here's what I have tried.
$('#hour').click(function() {
$('#clockHours').slideToggle();
$('#clockMinutes').hide();
});
$('#minutes').click(function() {
$('#clockMinutes').slideToggle();
$('#clockHours').hide();
});
//print clicked onto views
$('.timeSelect').click(function() {
var text = $( this ).text();
$('#hourView').val( text );
});
$('.minuteSelect').click(function() {
var text = $( this ).text();
$('#minuteView').val( text );
});I haven't had much luck figuring out how to improve the code. Here is a CodePen of the current working code and here's what I have tried.
Solution
There a few simple things you could do to help clean up this code. The first thing I always recommend is to create a closure for your script using an IIFE. This allows your code to run in its own scope and keeps everything out of the global scope. You can also pass in
The next thing you should consider is caching your selections. Since accessing the DOM is one of jQuerys slowest operations, you want to minimize this as much as possible. Anything you refer to more than once, you should cache.
I prefix all of my jQuery selections with $ so I know they are jQuery objects.
You should also start using
You might also want to consider DRYing your code. Since the click functions on both the
Here is an updated code sample based on the above. Note: I call an
Hope you find this helpful. Let me know if you have any questions.
jQuery to the function expression and safely refer to it as $.(function($, undefined ) {
// pass in undefined for very old (ES3) browsers
// code goes here
})( jQuery );The next thing you should consider is caching your selections. Since accessing the DOM is one of jQuerys slowest operations, you want to minimize this as much as possible. Anything you refer to more than once, you should cache.
var $min = $('#clockMinutes');
var $hr = $('#clockHours');I prefix all of my jQuery selections with $ so I know they are jQuery objects.
You should also start using
on for defining your events instead of the shorthand of click. Behind the scenes, click uses on so just skip the middle man.$min.on('click', function() {
//code here
});You might also want to consider DRYing your code. Since the click functions on both the
clockMinutes and clockHours are so similar, you can combine them and just pass in the "main" element you want to work on. For example:function updateScreen( $el ) {
$el.slideToggle();
if ( $el === $hour ) {
$min.hide();
} else {
$hour.hide();}
}
}Here is an updated code sample based on the above. Note: I call an
init function on document.ready to kick off the functionality.(function( $, undefined ) {
//place holder variables for our selections
var $min, $hour, $minView, $hrView;
function init() {
$min = $('#clockMinutes');
$hour = $('#clockHours');
$minView = $('#clockMinutes');
$hrView = $('#minuteView');
$('#hour').on('click', function() { updateScreen($hour); });
$(' #minutes').on('click', function() { updateScreen($min); });
$('.timeSelect').on('click', function() { updateView($hrView); });
$('.minuteSelect').on('click', function() {updateView($minView);});
}
function updateView( $view ) {
$view.val( $(this).text() );
}
function updateScreen( $el ) {
$el.slideToggle();
if ( $el === $hour ) {
$min.hide();
} else {
$hour.hide();
}
}
$(function() {
init();
});
})( jQuery );Hope you find this helpful. Let me know if you have any questions.
Code Snippets
(function($, undefined ) {
// pass in undefined for very old (ES3) browsers
// code goes here
})( jQuery );var $min = $('#clockMinutes');
var $hr = $('#clockHours');$min.on('click', function() {
//code here
});function updateScreen( $el ) {
$el.slideToggle();
if ( $el === $hour ) {
$min.hide();
} else {
$hour.hide();}
}
}(function( $, undefined ) {
//place holder variables for our selections
var $min, $hour, $minView, $hrView;
function init() {
$min = $('#clockMinutes');
$hour = $('#clockHours');
$minView = $('#clockMinutes');
$hrView = $('#minuteView');
$('#hour').on('click', function() { updateScreen($hour); });
$(' #minutes').on('click', function() { updateScreen($min); });
$('.timeSelect').on('click', function() { updateView($hrView); });
$('.minuteSelect').on('click', function() {updateView($minView);});
}
function updateView( $view ) {
$view.val( $(this).text() );
}
function updateScreen( $el ) {
$el.slideToggle();
if ( $el === $hour ) {
$min.hide();
} else {
$hour.hide();
}
}
$(function() {
init();
});
})( jQuery );Context
StackExchange Code Review Q#110458, answer score: 4
Revisions (0)
No revisions yet.