jQuery element reloader

Question

I have looked around for an already existing script for this, but couldn't find one, so i decided to make my own.

The point of this is to make it easy to reload an element, whether its a form or a link.

To use the script, you add a class to the link/form called "js_container-reloader". What the script does is it simply gets the content of the action url or href, creates a wrapper around the element you are reloading, takes the ajax response and puts it in to the wrapper, then removes the wrapper.

Its working as it should at the moment, but i wonder if there is a better way of doing this.

//Form element reloader
$(document).on("submit", ".js_container-reloader", function(e){
 e.preventDefault();
 //Call the reloader function
 elementReloader(e.currentTarget);
});
//Link element reloader
$(document).on("click", ".js_container-reloader", function(e){
 e.preventDefault();
 //Call the reloader function
 elementReloader(e.currentTarget);
});
/* Element reloader function
* Takes an element and the type of request this is */
function elementReloader(element){
 //Set variables
 var element = $(element),
 requestUrl,
 requestType,
 data,
 reloadContainer;
 //Wrap the element in a reloader div and save the div to a variable
 element.wrap("<div class='dv_dis-i js_reload-container'></div>");
 reloadContainer = element.closest('.js_reload-container');
 //If the element is a form
 if(element.is("form")){
 //Save the forms action
 requestUrl = element.attr('action');
 //Save the request type
 requestType = element.attr('method');
 //Save the forms data
 data = element.serialize();
 //If the request should be done by GET
 }else{
 //Get the forms action
 requestUrl = element.attr('href');
 //Save the request type
 requestType = "GET";
 }
 //Run the ajax script
 $.ajax({
 type: requestType,
 url: requestUrl,
 data: data,
 success: function(result){
 //If the result returned false, reload the alert container
 if(!result){
 reloadAlertContainer();
 //If its not false then load the container with the result
 }else{
 //Add the result to the wrapper
 reloadContainer.html(result);
 //Remvoe the wrapper
 reloadContainer.find(".js_container-reloader").unwrap();
 }
 },
 error: function(xhr) {
 /* Reload the alerts container
 * An alert is saved to the session if there was an error
 * with any script that was loaded.
 * This function reloads the container which shows the session alert. */
 reloadAlertContainer();
 },
 });
}

I don't assume my code is expert level as i am not an expert, but the goal is to make it as easy and dynamic as possible for future implementations. I tried looking in to combining the code, especially the first 2 functions for the event listeners, together in to one function, but couldn't find a way that seemed like a proper solution.

Just to clarify about the form posting - i do the form validation and checking in php, in the actual file that is being loaded, and not in JavaScript/jQuery. This script is just for reloading elements.

• Marc Rohloff: Personally I think you would be better switching on the element's type (FORM or A) rather than if a specific attribute. I would also get the method from the form's method aattribute. Also I notice that you pass in a type parameter but don't use it.
• K.D: Thank you! I updated the code and added the forms actual method (didn't think about forms having a GET option!), and i use the type now, i forgot i was even sending that. And what do you mean exactly by switching on the elements type? At what point in the code are you referring to?
• Sᴀᴍ Onᴇᴌᴀ: @MarcRohloff please add an answer instead of a comment. Refer to the section When shouldn't I comment? on Comment everywhere.
• Mast: @SᴀᴍOnᴇᴌᴀ Too late, the code is changed now. Answer invalidation and all that. But you're absolutely right that the comment should've been posted as an answer. Just, not now. Too late.
• K.D: @MarcRohloff I actually ended up changing the code even more in my script and switching the element types, there were issues with the way i was doing it, but now it seems to be much better and more dynamic. Thanks for that!

Answer 1

Sorry I was busier earlier. Here is a more complete review. Just a few things off the top of my head.

• Try and be consistent with your class naming. Instead of .js_container-reloader and js_reload-container use a common prefix, i.e. .js_reloader-trigger (or even just .js_reloader) and .js_reloader-container

• You can use a single call to the on function like:

  $(document).on("submit", ".js_container-reloader", ...

• var element = $(element), is syntactically incorrect since element is already defined as a parameter. One options is just use element = $(element); another pattern I often see is using a $ prefix on variables to indicate jQuery objects, i.e. var $element = $(element),

• requestUrl = element.attr('action'); would be better asrequestUrl = element.prop('action'); the latter will retrieve the default method for the form if no attribute is set.

• I personally think the use of wrap andunwrap` is an odd way to handle adding an element. I would remove the wrap and container code and just have something like the following on success:

  success: function(result){
   ....
   element.after(result);
   element.remove();
  

• I would add a .js_reloader-loading class to the element before starting the ajax query so that you can style it differently (fade it, add a spinner, etc).

• Lastly you probably want to implement some kind of logic to prevent multiple attempts at loading the form.

• K.D: Great suggestions, thanks! i have a few questions about some of the points: "You can use a single call to the on function like" - im not sure exactly what you mean, do you mean that instead of having two listeners, one for click and one for submit, i can have just the submit listener?
• K.D: And yes i agree about the wrapping and unwrapping, but if i use the .after() function, would there basically be a very shortly visible duplicate or the elements? For example, if i have a like button and i click it, wouldnt using .after() add the new button after it and then only remove the existing one, resulting in showing both buttons (even though it would just be momentary)?
• K.D: "Lastly you probably want to implement some kind of logic to prevent multiple attempts at loading the form." - i completely forgot about this when i was writing this code, i agree, i have to think about how this should be done, maybe when clicking on it, the current element should lose all click functionality, that way you wont be able to click on it again and trigger the script again before it reloads, or do you know of a better way?
• K.D: also, when you say to use requestUrl = element.prop('action'); do you mean to switch requestType = element.attr('method'); to: requestUrl = element.prop('method'); ?
• Marc Rohloff: Yes, you can use that syntax to listen to both events with a single listener function