3
\$\begingroup\$

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');
  });
\$\endgroup\$
2
\$\begingroup\$
  • 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.