1
\$\begingroup\$

I am trying to create a script that shows my login modal on page load. I have a button that opens the register modal and closes the login modal if clicked.

Here is the script I am using:

$(document).ready(function(){
  $('#loginmodal').modal('show')
});

$(".hidelogopenreg").click( function()
         {
            $('#loginmodal').modal('hide')
            $('#registermodal').modal('show')
         }
 );

Even though the works correctly, I fear it's using bad practices as I have little JavaScript knowledge and don't want to have to resolve problems further down the line.

Is this correct and efficient or is there a better way of writing this?

Another problem I had is that hiding a modal causes the body to shift to the right on close, to fix this I added:

body {
padding-right:0px !important;
margin-right:0px !important;
}

Could this also cause problems for me?

\$\endgroup\$
  • \$\begingroup\$ You're not allowed to make edits to your post that would invalidate existing answers. \$\endgroup\$ – cimmanon Jun 3 '15 at 21:02
  • \$\begingroup\$ I am not allowed to add additional information? \$\endgroup\$ – dan12 Jun 3 '15 at 23:20
  • \$\begingroup\$ Not if it makes the existing answers invalid. It looked to me like you'd copied code from the answer you'd already received. \$\endgroup\$ – cimmanon Jun 4 '15 at 0:35
  • \$\begingroup\$ Oh sorry, i changed it slightly and asked if it was okay but i forgot this is codereview and not stackoverflow. \$\endgroup\$ – dan12 Jun 4 '15 at 18:49
1
\$\begingroup\$

Could this also cause problems for me?

As with most CSS questions, the answer is maybe. It really depends on the rest of your CSS. Personally, I try to stay away from !important but I can certainly understand your usage. As long as it isn't causing additional issues, I don't see a problem with it.

Is this correct and efficient or is there a better way of writing this?

I would recommend wrapping your code in an IIFE so as not to pollute the global scope and give yourself a "safe" private scope. This also allows you to use $ for jQuery with no conflicts:

(function( $ ) {
  //code here 
})( jQuery );

Next, if you see where you use the same jQuery selection more than once, you should just go ahead and cache it:

var logModal = $('#loginmodal');

I would also recommend that you use K&R style braces instead of Allman style. This can help prevent issues you might encounter with JS.

$(".hidelogopenreg").click( function() {  // opening on same line K&R style
  $('#loginmodal').modal('hide')
  $('#registermodal').modal('show')
}

Lastly, you need to end your lines of code with a semicolon. You can get away with this in some situations but can cause issues later on. More information here and here.

  $('#loginmodal').modal('hide');  //end of line
  $('#registermodal').modal('show');  //end of line

Here is the code with these changes:

(function( $ ) {

  var $logModal = $('#loginmodal');
  var $regModal = $('#registermodal');
  var $hideLogin = $(".hidelogopenreg");

  function swapModals() {
    $logModal.modal('hide');
    $regModal.modal('show');
  }

  $(document).ready( function() {
    $logModal.modal( 'show');
    $hideLogin.on( 'click', swapModals );
  });

})( jQuery );

Hope that helps. Let me know if you have questions, comments or need clarification.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I edited my post to show my implementation. Would this work okay? \$\endgroup\$ – dan12 Jun 3 '15 at 20:46
  • \$\begingroup\$ Not sure what the correct way to do a follow up on CR. How about just create a fiddle. \$\endgroup\$ – Gary Storey Jun 3 '15 at 21:03
  • \$\begingroup\$ Oh wait sorry, i forgot this is codereview and not stackoverflow. My bad. \$\endgroup\$ – dan12 Jun 3 '15 at 23:24

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.