Plugin for WordPress and Foundation Reveal

Question

I figure that someone here probably knows a much better way to do this. I'm still figuring out AJAX and jQuery, so I consider that I still need to master quite a bit of knowledge.

My function extends Foundation 4 Reveal's functionality in a few ways:

• Uses WordPress AJAX to dynamically pull in content to populate a modal div.
• Centers the modal div and allows it to be variable width.
• Adds paging navigation from the modal window that when triggered will close the open modal window, then open the previous/next modal content.

While I feel like I've managed to figure out a lot on my own, my code isn't optimal; I would greatly appreciate any feedback or insights on ways to improve it, or things that I should avoid.

(function($) {
$.fn.displayPost = function() {
event.preventDefault();
var post_id = $(this).data("id");
var id = "#" + post_id;
// Check if the reveal modal for the specific post id doesn't already exist by checking for it's length
if($(id).length == 0 ) {
 // We'll add an ID to the new reveal modal; we'll use that same ID to check if it exists in the future.
 var modal = $('<div>').attr('id', post_id ).addClass('reveal-modal').appendTo('body');
 var ajaxURL = MyAjax.ajaxurl;
 $.ajax({
 type: 'POST',
 url: ajaxURL,
 data: {"action": "load-content", post_id: post_id },
 success: function(response) {
 modal.empty().html(response).append('<a class="close-reveal-modal">&#215;</a>').foundation('reveal', 'open');
 modal.bind('opened', function() {
 // Reset visibility to hidden and set display: none on closed reveal-modal divs, for some reason not working by default when reveal close is triggered on .secondary links 
 $(".reveal-modal:not('.reveal-modal.open')").css({'visibility': 'hidden', 'display' : 'none'})
 // Trigger resize 
 $(window).trigger('resize');
 return false;
 });
 }
 });
}
 //If the div with the ID already exists just open it.
 else {
 $(id).foundation('reveal', 'open');
 }
 // Recalculate left margin on window resize to allow for absolute centering of variable width elements
 $(window).resize(function(){
 var left;
 left = Math.max($(window).width() - $(id).outerWidth(), 0) / 2;
 $(id).css({
 left:left + $(window).scrollLeft()
 });
 });
}
 })(jQuery);
 // Apply the function when we click on the .reveal link
 // (document).ready won't work on any reveal-modal divs added subsequently
 // after page load via AJAX so use .on instead.
 jQuery(document).on("click", ".reveal,.secondary", function() {
 jQuery(this).displayPost();
 });
 // Close open modal via secondary paging links; target parent div id.
 jQuery(document).on("click", ".secondary", function() {
 var id = jQuery(this).closest("div").attr("id");
 jQuery(id).foundation('reveal', 'close');
 });

Answer 1

• L15, L26, L31-60: Inconsistent indentation levels.

• L4-43: Consider indenting this block.

• L04: I don't see the point in storing the ID rather than the jQuery object itself. Instead of var id = "#" + post_id;, go ahead and store the object: var $id = $("#" + post_id);. Modify the references as appropriate on L11 and L33.

• L13: Your code for appending the modal is nice in terms of the builder pattern, but it might be better just as:

  var modal = $("<div class='reveal-modal' id='" + post_id + "'></div>").appendTo("body")
  

• L14: There's no point in declaring the ajaxURL variable. Instead, just go ahead and use { ... url: MyAjax.ajaxurl, ... } on L17,

• L20: The jQuery API documentation on .html() notes a specific case in which it's important to use .empty().html(). I don't believe this applies in your case; you don't need the call to .empty() since .html() will effectively empty the container first.

• L21: Since jQuery 1.7, .on() is preferred.

• L58-59: This part is at best senseless and wrong, and at worst unintuitive and potentially deceptive. By extracting the ID attribute and using it as a selector, you're effectively matching against the ID without the #. This either means that it doesn't do what you think it does, in which case you should use:

  jQuery(this).closest("div").foundation("reveal", "close");
  

  or you for some reason have a construct like <div id='span'> and you really mean to match all span elements instead of #span. In that case, I strongly suggest you restructure your HTML to be more semantically sensible, and to leave a comment explicitly detailing that that's your intention.