Modal onclick not working

JavaScript
1

I am trying to build a Wordpress plugin that will add a login modal for logged out users.

I’m using the code below.

I can verify that the modal html is being added to the site, and if I manually change the modal to “display:block” it works. However, I cannot get the button to open the modal to work. I’ve tried adding the JS to the site by just manually adding it to a page, and it still won’t work.

I get no console errors, and if I manually type it into the console, it still won’t work.

Link to staging site

/* CREATE THE USER MENU LOCATION */

function cg_user_menu_location() {
    register_nav_menu('cg-user-menu',__( 'User Menu' ));
  }
  add_action( 'init', 'cg_user_menu_location' );


/* ADD USER SHORTCODE */
function get_cg_user_menu(){
    if ( is_user_logged_in() ) {
echo '<div id="cg-user-menu-wrap">';
echo get_user_meta( get_current_user_id(),'first_name','true');
wp_nav_menu(array(
    'menu' => 'User Menu',
    'menu_class' => 'cg-user-menu',
)); 
echo '</div>';
    }
    else {
        echo '<button class="loginModalOpen"> Login / Join </button>';
    }
};
function cg_user_menu() { 
    $menu = get_cg_user_menu();
    return $menu;
    }; 
    add_shortcode('cg_user_menu', 'cg_user_menu'); 

/* ADD THE LOGIN FORM POPUP TO FOOTER IF NO USER LOGGED IN */
add_action('wp_footer','add_login_modal');
function add_login_modal() {    
if (! is_user_logged_in() ){
    echo '
    <div id="loginModal" class="modal">
      <div class="modal-content">
        <span class="close">&times;</span>
        ';
        wp_login_form();
   echo '</div></div>';

    echo '<script>
    var loginModal = document.getElementById("loginModal");
    var loginModalOpen = document.getElementsByClassName("loginModalOpen");
    var loginModalClose = document.getElementsByClassName("loginModalClose");

    loginModalOpen.onclick = function(){
        loginModal.style.display = "block";
    }

    loginModalClose.onclick = function(){
        loginModal.style.display = "none";
    }
    </script>';
};

};

The first part of the code is to create a user menu I can add to the header.

2

Hi @jeremy58, document.getElementsByClassName() returns a HTML collection, not just a single DOM element (note the plural); and assigning an onlick property to such a collection simply has no effect, so you’d actually need to set loginModalOpen[0].onclick.

However, better yet would be to:

  1. Use document.querySelector() instead since there is only a single element you need anyway, and
  2. Use addEventListener() instead of directly assigning a handler function, which would throw an error if the target has not implemented this (and also has other advantages)
var loginModal = document.getElementById('loginModal')
var loginModalOpen = document.querySelector('.loginModalOpen')

loginModalOpen.addEventListener('click', function () {
  loginModal.style.display = 'block'
})

BTW I’d strongly suggest not to echo any JS yourself – this can rather quickly become a mess that’s hard to read, debug and maintain. Instead, put it in a separate JS file and enqueue it using WP’s script registry… this is especially important if you have dependencies such as jQuery, but should always be used anyway for good measure. :-)

3

Thanks for the help!

Is the querySelector method still a good one to use if there are multiple elements (all identified by .loginModalOpen) on a page that have the class?

For example, I will be using a paywall plugin and I’d like to add to the “copy” an element that can also open the modal, so there would be pages that have multiple .loginModalOpen elements.

With respect to enqueue vs echo: that seems sort of obvious now that you say it. That’s how I’ve done it when working with theme files, it never occured to me to do it with plugin javascript.

wp_enqueue_script( 'cg_login_js', plugin_dir_url( __FILE__ ) . 'js/cg-login.js' );

should do it.

4

In that case you might actually use getElementsByClassName() and iterate over the elements in a regular for loop… or, use querySelectorAll() which returns a node list that (unlike HTML collections as returned by the former) has a forEach() method like arrays that avoids closure issues when adding event listeners:

var loginModal = document.getElementById('loginModal')
var openButtons = document.querySelectorAll('.loginModalOpen')

openButtons.forEach(function (loginModalOpen) {
  loginModalOpen.addEventListener('click', function () {
    loginModal.style.display = 'block'
  })
})

Personally I prefer to always use querySelectorAll() anyway as it allows for arbitrary CSS selectors, similar to jQuery’s sizzle engine… if I’m not mistaken, jQuery has actually been the inspiration for this method.

Yup that’s the way. :-)

5

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.