As I learn jQuery I choose a little project as often as I have time and attempt to make it on my own. I have spent the past little while creating a customised select element and would love to hear how it could be improved. Here is the fiddle: http://jsfiddle.net/joshnh/buQr9/
Hmm, another great point, but I'm not entirely sure on how to implement it. Simply using .stop() before sliding the list up or down leads to a strange situation where it won't show it all if you click on the div a few times in quick succession.
So I discovered a major flaw: as soon as I had more than one select element, the whole thing broke. It has now been fixed and the latest version is here: http://jsfiddle.net/joshnh/fyqdS/
It's pretty good practice to add a $ to the beginning of jQuery object variables. So:
// Cache the select form element/s var $select = $('select'), selectHeight = $select.height(), numberOfOptions = select.children('option').length;
You don't have to do that though - It just seems to be common practice now and I do it I think it's best to keep CSS out of javascript completely. Rather add a class.
// Hides the select element $select.addClass('hidden');
This isn't really good practice:
// Insert an unordered list after the styled div styledSelect.after('<ul class="options"></ul>'); var ul = $('ul.options');
It's better to do something like:
var $listOptions = $('<ul />', { 'class' : 'options' }).insertAfter($styledSelect);
It's not really scalable or neat to just add strings of stuff so it's better to create and cache jQuery objects to a variable that way. makes your code much neater too.
Your way:
// Insert a list item into the unordered list for each select option for(i = 0; i < numberOfOptions; i++) { ul.append('<li>' + select.children('option').eq(i).text() + '</li>'); }
Better way:
for(var i = 0; i < numberOfOptions; i++) { $('<li />', { text: $select.children('option').eq(i).text() }).appendTo( $listOptions); }
remember to add var i otherwise you are setting the global i value which isn't what you want to do there.
@tobeeornot I only use the jQuery .each() function if I have to. It's easy to do a quick javascript loop which is much lighter on the browser.
@jamy_za Wow! Thank you so much mate, that is incredibly helpful! I have implemented your changes (after making sure I understand them), and the ones recommend by @TheDoc, and this is where it is currently at: http://jsfiddle.net/joshnh/eZzv7/
Is this the best way to close any other select that may be open when clicking on a new one?
for(var i = 0; i < numberOfSelects; i++) { if($('div.styledSelect').eq(i).hasClass('active') === true) { $('div.styledSelect').eq(i).removeClass('active') .next('ul.options').filter(':not(:animated)').slideUp(250); } }
Also, is it best practise to explicitly state '=== true', or can I leave that off?
Oh, and if you look towards the bottom of the code, you will see that I am having difficulty assigning the the clicked list item to the value of the select. Any suggestions?
EDIT: I think I have solved that last issue by assigning $(this) to the variable $this.
You shouldn't have to cycle through all of the styledSelects, rather it would be more efficient to just use a jQuery selector to find them (demo):
// Show the unordered list when the styled div is clicked (also hides it if the div is clicked again) $styledSelect.click( function(e) { e.stopPropagation(); $('div.styledSelect.active').each(function(){ $(this) .removeClass('active') .next('ul.options').filter(':not(:animated)').slideUp(250); }); $(this).toggleClass('active') .next('ul.options').filter(':not(:animated)').slideToggle(250); });
Also I wouldn't use === true because it isn't necessary.
Oh and I also wanted to add a comment to @jamy_za's suggestion in that the following isn't good practice:
// Insert an unordered list after the styled div styledSelect.after('<ul class="options"></ul>');
I would use this format in a plugin just in case the user is using older versions of jQuery. The ability to include an object with parameters wasn't added until jQuery 1.4, and some people are still stuck using older versions.
@Mottie I'm still learning when it comes to the wonderful world of jQuery, but isn't using a for loop the better way to go? I've done some performance research and each() seems like a naughty tool.
@TheDoc It would be if there were a lot of items to iterate through or if the each is called many times, say like on mouse move or window scroll. See this jspref example.
But in this case, there are only two styledSelect divs; but even with 20 or 50, you probably won't notice any performance issues.
Thanks Rob, I'm not a fan of the way it handles clicks while the animation is running though. I agree that it would work well for slower animations. With the speed I am running the animation at there is no way a 'rapid clicker' could know what he wanted to click on before the animation was complete.
Thanks for raising the performance issue too Gray, if I was to ever release this as a plugin, I would use a for loop.
@joshuanhibbert Hmm, that test does add an additional function, changing the background color, so I'm just not sure.
And now that I think about it, the recommendations from tip #1 above, shows how to optimize use of the sizzle engine which only activates when you use multiple selectors... so yeah, it's not relevant in this case.
So at this point, I removed the background color from the jsperf and the class name only was faster, then I copied this post into the HTML to see if it's faster with returning just one element and well the class name only was still faster... so yeah @jamy_za and you were right!
I hereby retract my last statement ;)... you learn something new everyday :P
Thanks in advance!
1. Click anywhere to close. I shouldn't have to click on the select button again to close the choices.
2. A little dropdown icon on the select might be nice, little UI enhancement.
3. The sliding is a little bit slow to me.
The other thing you'll want to do is make sure the user can't queue up a bunch of animations.
.stop()before sliding the list up or down leads to a strange situation where it won't show it all if you click on the div a few times in quick succession.EDIT: Chris has a solution on this very site: http://css-tricks.com/full-jquery-animations/
Here's another one for you: if you have one dropdown open, when you click on a new one it should close the previous one.
When you click any select button, you should loop through all of them and see if any of them have an 'active' state. If they do, close it!
For the record, my favourite animal is giraffe
You don't have to do that though - It just seems to be common practice now and I do it
I think it's best to keep CSS out of javascript completely. Rather add a class.
This isn't really good practice:
It's better to do something like:
It's not really scalable or neat to just add strings of stuff so it's better to create and cache jQuery objects to a variable that way. makes your code much neater too.
Your way:
Better way:
remember to add var i otherwise you are setting the global i value which isn't what you want to do there.
@tobeeornot I only use the jQuery .each() function if I have to. It's easy to do a quick javascript loop which is much lighter on the browser.
For the record, I'm not a cat person.
I like wombats!
Is this the best way to close any other select that may be open when clicking on a new one?
Also, is it best practise to explicitly state '=== true', or can I leave that off?
Oh, and if you look towards the bottom of the code, you will see that I am having difficulty assigning the the clicked list item to the value of the select. Any suggestions?
EDIT: I think I have solved that last issue by assigning $(this) to the variable $this.
You shouldn't have to cycle through all of the styledSelects, rather it would be more efficient to just use a jQuery selector to find them (demo):Also I wouldn't use
=== truebecause it isn't necessary.But in this case, there are only two styledSelect divs; but even with 20 or 50, you probably won't notice any performance issues.
This works better for slower animation or rapid clickers. The dropdowns will now close instead of remaining open.
Thanks for raising the performance issue too Gray, if I was to ever release this as a plugin, I would use a for loop.
Is the div in div.styleSelect necessary? Each thing you add there slows down the selecting process. So it would be faster to go $('.styleSelect')
And now that I think about it, the recommendations from tip #1 above, shows how to optimize use of the sizzle engine which only activates when you use multiple selectors... so yeah, it's not relevant in this case.
So at this point, I removed the background color from the jsperf and the class name only was faster, then I copied this post into the HTML to see if it's faster with returning just one element and well the class name only was still faster... so yeah @jamy_za and you were right!
I hereby retract my last statement ;)... you learn something new everyday :P