Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm using a lightweight jQuery popup plugin called 'bPopup'. I'm using it on my website at the moment to load multiple popup windows when clicked. I was recently told that my code was inefficient as I was loading multiple popups with multiple JavaScript 'listeners', i.e.:

<script type="text/javascript">
;(function($) {
$(function() {
    $('#my-button_1').bind('click', function(e) {
        e.preventDefault();
        $('#element_to_pop_up_32754925023').bPopup();
    });
});
})(jQuery);
</script>

<script type="text/javascript">
;(function($) {
$(function() {
    $('#my-button_2').bind('click', function(e) {
        e.preventDefault();
        $('#element_to_pop_up_95031153149').bPopup();
    });
});
})(jQuery);

^^ The multiple JavaScript 'listeners'. And, for the Popups:

<!-- Button that triggers the popup -->
<a class="main" id="my-button_1" href="#">Popup 1</a></b><br />
<!-- Element to pop up -->
<div id="element_to_pop_up_1">
// ...
</div>


<!-- Button that triggers the popup -->
<a class="main" id="my-button_1" href="#">Popup 1</a></b><br />
<!-- Element to pop up -->
<div id="element_to_pop_up_1">
// ...
</div>

He's probably right (sure of it), but not sure how to implement this, or whether this is even possible (small chance he's wrong).

Help? And thanks!

share|improve this question
add comment

2 Answers

up vote 1 down vote accepted

He is right. Think about maintenance.

If you wanted to add another link that popped up another element you would need to add yet another listener function. Removing one would be the same thing. Too many things to keep track of.

Change the code to add the event according to a class name and then you can bind to all at once. Then include the popup element ID within the attributes of the opener link.

$('.popup-opener').bind('click', function(e) {
    e.preventDefault();
    $($(this).attr('popup-element')).bPopup();
});

<!-- Button that triggers the popup -->
<a class="main popup-opener" popup-element="element_to_pop_up_1" id="my-button_1" href="#">Popup 1</a></b><br />
<!-- Element to pop up -->
<div id="element_to_pop_up_1">
// ...
</div>

Better yet, use the on() function to bind the event. http://api.jquery.com/on/

Adding new openers now requires no changes to the javascript.

share|improve this answer
    
Yeah, figured it out yesterday (I'm a complete amateur with JS), thanks anyway!! :) –  Danny Noam Feb 15 '13 at 14:01
add comment

The below function ( myFunction() ) takes the Id of anchor/div tag which is clicked and another id of div content to be display. And applies the same style for all popup models. And also it hides the old popup which already opened when u open new popup. All popup properties you can change.

Here i used only for two popups but you can use it for many as same did here.

<script type="text/javascript">

function myFunction(whId,whtDivContent,e) {
    //var totWidth = $(document).width();
    //var marTop = position.top;
    var elt = $(whId);
    var position = elt.position();
    var marLeft = position.left - 130;

    if(marLeft <= 1) {
        marLeft = 10;
    }

    var openModal_profile ='#openModal_profile';
    var openModal_menu ='#openModal_menu';

    // Prevents the default action to be triggered. 
    e.preventDefault();

    $(whtDivContent).bPopup({
        position: [marLeft, 0] //x, y
        ,opacity: 0.9
        ,closeClass : 'b-close'
        ,zIndex: 2
        ,positionStyle: 'fixed' //'fixed' or 'absolute' 'relative'
        ,follow: [false,false] //x, y
        ,onOpen: function() {
            if(openModal_profile == whtDivContent) {
                $(openModal_menu).bPopup().close();
            }
            else if(openModal_menu == whtDivContent) {
                $(openModal_profile).bPopup().close();
            }

            $(whId).css({'background-color':"#DFDFDF"}); 
        }
        ,onClose: function() { $('.close').click(); $(whId).css({'background-color':""}); }

    });
}


;(function($) {
    // DOM Ready
    $(function() {
       // From jQuery v.1.7.0 use .on() instead of .bind()
       //$(id_menu).on('click',function(e) {}

       var id_menu = '#id_menu';
       var openModal_menu ='#openModal_menu';
       $(id_menu).toggle(function(e) {
           //$(id_menu).css({'background-color':"#DFDFDF"});
           myFunction(id_menu,openModal_menu,e);
       },function(e){
           //$(id_menu).css({'background-color':""});
           $('.close').click();
           $(openModal_menu).bPopup().close();

       });

       var id_profile = '#id_profile';
       var openModal_profile ='#openModal_profile';
       $(id_profile).toggle(function(e) {
           //$(id_profile).css({'background-color':"#DFDFDF"});
           myFunction(id_profile,openModal_profile,e);
       },function(e){
           //$(id_profile).css({'background-color':""});

           $(openModal_profile).bPopup().close();
       });

    //ENDS HERE    
    });
})(jQuery);    


</script>
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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