patternjavascriptMinor
Fading and Loader jQuery Improvement
Viewed 0 times
loaderimprovementfadingjqueryand
Problem
I'm not much of a jQuery coder (more of a PHP coder), except to call a few get values out of class and id and Ajax calls. However, I need to make a checkout system work and one thing I would like to do is to put a loading wait while I send a form through Ajax. When it's finished, it would redirect the user.
So on click:
Here is my code, following why I don't like it:
Let's get a few questions out of the way:
My issue (and feeling) that this can be better. I am using CSS with display:none to hide my two div, and then I call
So on click:
- load the loading spinner
- fade the background out
- Ajax call
- when Ajax is done submitting a form which indirectly redirects.
Here is my code, following why I don't like it:
jQuery.fn.center = function () {
this.css("position","absolute");
this.css("top", ( $(window).height() - this.height() ) / 2+$(window).scrollTop() + "px");
this.css("left", ( $(window).width() - this.width() ) / 2+$(window).scrollLeft() + "px");
return this;
}
$( ".buttonFinish" ).on('click', function(event) {
$('body').fadeOut('slow');
$('#loader').center();
$('#loading').center();
$('#loader').show();
$('#loading').show();
//Use the above function as:
var request = $.ajax({
url:'{{ URL::route('saveCart') }}',
type:'POST'
});
request.done(function(msg) {
$('.order_number').val(msg);
$('#summaryForm').submit();
event.preventDefault();
});
return false;
});Let's get a few questions out of the way:
{{UR::route('')}} is laravel syntax to call the routing name for URL. loader/loading are two div that I have for the spinner one is text the other is an animation through CSS. (I'd rather CSS it than to load a pix, but I am aware that loader.gif can be small)My issue (and feeling) that this can be better. I am using CSS with display:none to hide my two div, and then I call
fadeOut, show, show, and center center function to center my loading Ajax in oSolution
Let's go through it one section at a time.
-
You don't have to explicitly append
correctly format your CSS styles.
-
Since this is a jQuery plugin, you
it preserves chaining.
-
If the
styling the
-
As mentioned by @Flambino, this particular code (as presented in the question) will fade out the entire body of the HTML document, which includes everything, even the
-
It's best practice to enclose JavaScript that uses external dependencies inside an IIFE.
By doing so, you can use the "safe" name of an external dependency while still being able to
alias is to a more convenient identifier locally. In addition, you can freely create variables inside the scope
of the IIFE without polluting the global namespace. Your example code is fairly short, but you likely have much
more code on the page, and it is a good habit to have regardless. Here's one way to do it:
-
You can select both elements at the same time, just like in CSS:
-
You can chain the
-
Since you're not using the
you can skip declaring it and simply chain
As an overall note, the indentation of the code (as it is in the question) seems haphazard,
making the control flow more difficult to see than it could be. Making matching
braces have the same indentation (and, in general, maintaining a consistent style)
will help make the code more readable and understandable.
jQuery.fn.center = function () {
this.css("position","absolute");
this.css("top", ( $(window).height() - this.height() ) / 2+$(window).scrollTop() + "px");
this.css("left", ( $(window).width() - this.width() ) / 2+$(window).scrollLeft() + "px");
return this;
}-
You don't have to explicitly append
'px' to your numeric values. jQuery is smart enough to correctly format your CSS styles.
-
Since this is a jQuery plugin, you
return this; at the end of the function. This is good, asit preserves chaining.
-
If the
.center function is only used for the loading divs, consider removing it and insteadstyling the
divs with plain CSS.#loader {
position: fixed;
top: 50%;
left: 50%;
margin-top: -50px; /* half of the height */
margin-left: -100px; /* half of the width */
}
...$( ".buttonFinish" ).on('click', function(event) {
$('body').fadeOut('slow');
$('#loader').center();
$('#loading').center();
$('#loader').show();
$('#loading').show();-
As mentioned by @Flambino, this particular code (as presented in the question) will fade out the entire body of the HTML document, which includes everything, even the
loader elements. Perhaps the code that you're actually using is slightly different (is the selector .body or #body instead?).-
It's best practice to enclose JavaScript that uses external dependencies inside an IIFE.
By doing so, you can use the "safe" name of an external dependency while still being able to
alias is to a more convenient identifier locally. In addition, you can freely create variables inside the scope
of the IIFE without polluting the global namespace. Your example code is fairly short, but you likely have much
more code on the page, and it is a good habit to have regardless. Here's one way to do it:
(function($) {
$( ".buttonFinish" ).on('click', function(event) {
...
});
})(jQuery);-
You can select both elements at the same time, just like in CSS:
$('#loader, #loading').-
You can chain the
.show() and .center() together: $('...').show().center().//Use the above function as:
var request = $.ajax({
url:'{{ URL::route('saveCart') }}',
type:'POST'
});
request.done(function(msg) {
$('.order_number').val(msg);
$('#summaryForm').submit();
event.preventDefault();
});
return false;
});-
Since you're not using the
request variable for anything other than .done(),you can skip declaring it and simply chain
.done() after $.ajax():$.ajax({
...
}).done(function(msg) {
...
});As an overall note, the indentation of the code (as it is in the question) seems haphazard,
making the control flow more difficult to see than it could be. Making matching
braces have the same indentation (and, in general, maintaining a consistent style)
will help make the code more readable and understandable.
Code Snippets
jQuery.fn.center = function () {
this.css("position","absolute");
this.css("top", ( $(window).height() - this.height() ) / 2+$(window).scrollTop() + "px");
this.css("left", ( $(window).width() - this.width() ) / 2+$(window).scrollLeft() + "px");
return this;
}#loader {
position: fixed;
top: 50%;
left: 50%;
margin-top: -50px; /* half of the height */
margin-left: -100px; /* half of the width */
}
...$( ".buttonFinish" ).on('click', function(event) {
$('body').fadeOut('slow');
$('#loader').center();
$('#loading').center();
$('#loader').show();
$('#loading').show();(function($) {
$( ".buttonFinish" ).on('click', function(event) {
...
});
})(jQuery);//Use the above function as:
var request = $.ajax({
url:'{{ URL::route('saveCart') }}',
type:'POST'
});
request.done(function(msg) {
$('.order_number').val(msg);
$('#summaryForm').submit();
event.preventDefault();
});
return false;
});Context
StackExchange Code Review Q#45717, answer score: 3
Revisions (0)
No revisions yet.