Jump to content

Can someone check my JQuery?

Joss

By Joss,
in Dev Talk

 Share

Recommended Posts

Joss Hero Member

Joss

Posted
Posted

I am developing a PW site where the music is loaded by jquery and ajax.

I have written my own JQuery script as a test (Yes, ME! Written a script!!) 

The test is actually working (no errors in the console), but would some kind person just look over the script and see whether I have done something idiotic and can be improved?

I have never written a proper jquery script before.

Basically, when you click on an "open player" button, the script loads a page into a hidden div then slides it open, giving the div a "player-open" class in the process.

The loaded page includes a Close Player button. When you click that, it slides the div closed, empties it and hides it again, removing the "player-open" class in the process.

PW loops through a pile of these, so you get several Open Player buttons on the page. Each has a unique ID and also has a unique DIV associated with them (I just use the linked-to page id to help make them unique). 

So, to allow for this, the script also checks to see if any there are any visible divs with the "player-open" class. If there is, it slides them closed before opening the new one.

It is pretty much like an accordion, but I can shove the elements where I want on the page - they dont need to be grouped together.

it is also easy to change it so all of the links use the same DIV rather than their own unique div.

Anyway, here is the script

<!DOCTYPE html>
<html>
<head>
<script src="//ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<script>
$(document).ready(function(){
	$.ajaxSetup ({
    // Disable caching of AJAX responses
    cache: false
	})
	$('.openplayer').click(function() {
	event.preventDefault(); // stop the browser from following the link  

	  	var url = $(this).attr('href');
	  	var divclass = $(this).attr('data-class');
	  	var buttonclass = $(this).attr('id');
		//$("div").removeClass("player-open"); - doesn't seem to be needed.

		/*  
		Check to see if there is a div with the class player-open
		If there is, slide it closed, remove its player-open class 
		then move on to the load
		*/
		if ($(".player-open").is(':visible')) { 
			$(".player-open").slideUp("slow", function(){
				$(".player-open").empty().removeClass("player-open");
	  			$("#" + divclass).load(url, function(){
	  			    $("#" + divclass).hide().slideDown("slow");
				    $(this).addClass("player-open");
	  	                }); 				
			});			
		}else{
		/*
		If there is not a visible div with the class .player-open
		Then just go ahead with the load
		*/
	  	$("#" + divclass).load(url, function(){

	  		$("#" + divclass).hide().slideDown("slow");
			$(this).addClass("player-open");
			
	  	});
	  	} 
	}); //end openplayer

});
/* This is for the close player button that is part of the called html file
It first slides the div back up and then empties it for good measure */
$(document).ajaxComplete(function() {
	$('.closeplayer').click(function() {		
		$(".player-open").slideUp("slow", function(){
			$(".player-open").empty().hide();
			$("div").removeClass("player-open");	
		})	
	});
});
</script>
</head>
<body>

<!--
ProcessWire will use page id to customise the id of the button and div in each case, making each unique
-->
<button><a href="ajaxtext1.html" data-class="div1" class="openplayer" id="button1">Open Player</a></button><br>
<div id="div1" style="display: none"></div>

<button><a href="ajaxtext2.html" data-class="div2" class="openplayer" id="button2">Open 2 Player</a></button><br>
<div id="div2" style="display: none"></div>

<button><a href="ajaxtext3.html" data-class="div3" class="openplayer" id="button3">Open 3 Player</a></button><br>
<div id="div3" style="display: none"></div>

<button><a href="ajaxtext4.html" data-class="div4" class="openplayer" id="button4">Open 4 Player</a></button><br>
<div id="div4" style="display: none"></div>

</body>
</html>

Thanks!

Link to comment
Share on other sites
MadeMyDay Sr. Member

MadeMyDay

Posted
Posted

Hi Joss,

looks good. But I wouldn't empty() the loaded content, just hide it. And before loading (new) content first check if it already has been loaded, then prevent loading it again and just display it. Doesn't cost you a lot of rewriting but your users will thank you for not loading the same content more often as needed ;)

Link to comment
Share on other sites
Joss Hero Member

Joss

Posted
  • Author
Posted

Hi! MadeMyDay

The reason I am emptying the content is that these will be large(ish) mp3 and ogg files in the <audio> tag. if someone closes one or chooses another one, I want to make sure that the file not only stops playing, but stops loading too - I could have 100 of these on a page!

Link to comment
Share on other sites
MadeMyDay Sr. Member

MadeMyDay

Posted
Posted

Seems legit. If you use a plain HTML5 solution it is sufficient to set the src of the file to "", the browser stops downloading it. When clicked again, you can set the src again and the browser starts downloading again. But I think this is no big benefit for your project (one less request), so I think your solution is fine.

Link to comment
Share on other sites
Joss Hero Member

Joss

Posted
  • Author
Posted

Thanks.

Because this particular site sits on a shared server. I have to be wary of resources - the current site has had problems with files sometimes not playing the whole way through because the player outpaces the file loading on a bad day. This tends to be made a lot worse if there are other players on the page.

Link to comment
Share on other sites
Torsten Baldes Sr. Member

Torsten Baldes

Posted
Posted

looks good and as long as it works …

i tried to optimize some things (one could do even more):

$(document).ready(function(){
	$.ajaxSetup ({
		// Disable caching of AJAX responses
		cache: false
	});

	$('body').on('click', '.openplayer', function() { // delegate bind event
		event.preventDefault(); // stop the browser from following the link  

		var $this = $(this), // cache selector
			playerOpenClass = "player-open", // we use this as variable for all other occurences
			$playerOpen = $("."+playerOpenClass), // cache selector
			url = $this.attr('href'),
			divclass = $this.attr('data-class'),
			$playerDiv = $("#" + divclass), // cache selector
			buttonclass = $this.attr('id'); // 
 
		/*  
		Check to see if there is a div with the class player-open
		If there is, slide it closed, remove its player-open class 
		then move on to the load
		*/
		if ($playerOpen.is(':visible')) { 
			$playerOpen.slideUp("slow", function(){
				var $this = $(this);
				$this.empty().removeClass(playerOpenClass);

				$playerDiv.load(url, function(){
					var $this = $(this);
					$this
						.hide()
						.slideDown("slow")
						.addClass(playerOpenClass);
				}); 				
			});			
		}else{
		/*
		If there is not a visible div with the class .player-open
		Then just go ahead with the load
		*/
			$playerDiv.load(url, function(){
				var $this = $(this);
				$this
					.hide()
					.slideDown("slow")
					.addClass(playerOpenClass);
			}); 
		} 
	}) //end openplayer
	.on('click', '.closeplayer', function() { // delegate bind close event
		$playerOpen.slideUp("slow", function(){
			var $this = $(this);
			$this
				.empty()
				.hide()
				.removeClass("player-open");	
		})	
	});
});

i just edited, but didn't test it, so beware of typos.

  • Like 4
Link to comment
Share on other sites
Joss Hero Member

Joss

Posted
  • Author
Posted

Oh, I will have a look through that.

One thing I notice is that you have moved the close player function into the document ready function.

This button gets loaded with the external page and for me it didn't work unless I had it in ajaxComplete.

Link to comment
Share on other sites
diogo Hero Member

diogo

Posted
Posted

Went through it very quickly, wrote down a couple of improvements:

Every time you call $(this), all the jQuery function runs and adds extra overhead, the best practice is to assign it to a variable:

var $this = $(this); //<- here. I usually use a $ to mark the variables that represent a jQuery collection
var url = $this.attr('href');
var divclass = $this.attr('data-class');
var buttonclass = $this.attr('id');

The same applies to elements:

var $playerOpen = $(".player-open");
var $element = $("#" + divclass);
$playerOpen.slideUp("slow", function(){
  $playerOpen.empty().removeClass("player-open");
  $element.load(url, function(){
    $element.hide().slideDown("slow");
    $(this).addClass("player-open");
  }); // EDIT: that one was missing. not easy to write code in the forum editor 
}); 

You  could, instead of connecting the buttons and the divs by ID , simply open the next element, since that's how it's organized:

//$("#" + divclass).load(url, function(){
$this.next().load(url, function(){

edit: tobaco was faster and more complete :)

  • Like 3
Link to comment
Share on other sites
Torsten Baldes Sr. Member

Torsten Baldes

Posted
Posted

One thing I notice is that you have moved the close player function into the document ready function.

This button gets loaded with the external page and for me it didn't work unless I had it in ajaxComplete.

the way it's bound right now, it will also work, when the element is not yet loaded.

basically it says, if someone clicks anywhere on the body, check if the target of the click has a class ".closeplayer".

if yes do what you have to do, if no, just carry on.

this is what the jQuery documentation says:

Delegated events have the advantage that they can process events from descendant elements that are added to the document at a later time. By picking an element that is guaranteed to be present at the time the delegated event handler is attached, you can use delegated events to avoid the need to frequently attach and remove event handlers. This element could be the container element of a view in a Model-View-Controller design, for example, or document if the event handler wants to monitor all bubbling events in the document. The document element is available in the head of the document before loading any other HTML, so it is safe to attach events there without waiting for the document to be ready.

In addition to their ability to handle events on descendant elements not yet created, another advantage of delegated events is their potential for much lower overhead when many elements must be monitored.

  • Like 2
Link to comment
Share on other sites
Joss Hero Member

Joss

Posted
  • Author
Posted
You  could, instead of connecting the buttons and the divs by ID , simply open the next element, since that's how it's organized:

It might not be organised that way - actually, it probably wont on one page - the individual links will scattered.

This was part of why I didn't adapt an accordion - I wanted to have individual players scattered, while making sure only one was opened and loading at anyone time.

As for everything else - thanks guys, I am neatening it up!

Link to comment
Share on other sites
 Share
Go to topic listing

  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...