2
\$\begingroup\$

I have made a password reveal feature, however the code is duplicated. Essentially all I want to be able to improve the code, however I'm not exactly sure how I could do this, all I know is it would consist of some form of loop.

I would go straight to jQuery but there is no need in 2018.

This is my functionality markup for my HTML (it’s the same for the login page and modal):

<form action="/login" method="post">

    <div class="input-group">                             
        <label style="display: none" for="username">Username :</label>
        <input class="input-field width-100" type="text" autocomplete="username" name="username" placeholder="Enter Your Username">
    </div>

    <div class="input-group">
        <label style="display: none" for="password">Password :</label>
        <input class="input-field width-100 password" data-js="viewShowPassword" type="password" autocomplete="current-password" name="password" placeholder="Enter Your Password">
    </div>

    <div class="input-group--flex">
        <div class="remember-me">
            <input type="checkbox"/> Remember Me
        </div>

        <div class="show-password">
            <a class="text-dark-green--dark" data-js="viewPasswordReveal">Show Password</a>
        </div>
    </div>              

    <div class="input-group">
        <button class="submit width-100 input-field text-white background-red--light" style="padding: 5px">Login</button>
    </div>
</form>

And this is my JavaScript, which shows both the login page and login modal code to demonstrate what I have so far:

document.addEventListener("DOMContentLoaded", function(event) {

    var modalShowPassword = document.querySelector("[data-js=modalPasswordReveal]");
    var modalPassword = document.querySelector("[data-js=modalPassword]");

    if(modalShowPassword || modalPassword) {
        modalShowPassword.addEventListener("click", function (){
            if (modalPassword.type === "password") {
                modalPassword.type = "text";
            } else {
                modalPassword.type = "password";
            }
        });
    }


    var viewShowPassowrd = document.querySelector("[data-js=viewPasswordReveal]");
    var viewPassword = document.querySelector("[data-js=viewShowPassword]");

    if( viewShowPassowrd || viewPassword) {
        viewShowPassowrd.addEventListener("click", function (){
            if (viewPassword.type === "password") {
                viewPassword.type = "text";
            } else {
                viewPassword.type = "password";
            }
        });
    }
});

FYI I'm fairly new to the world of JavaScript so I'm not very good.

\$\endgroup\$
2
\$\begingroup\$

Before I comment on your implementation. Your code has a bug in it.

The code inside this if block:

if(modalShowPassword || modalPassword) {

will execute if either modalShowPassword or modalPassword is set but you really only want it to execute if both are set. You want to use the and (&&) operator like this:

if(modalShowPassword && modalPassword) {

With your code, think about what would happen if you were to have more (say 10) password boxes on your page? Would you repeat your code 10 times with different variables and different values for the data-js attribute? What I would suggest is to use something similar to the Bootstrap style (see how their modal dialogs work here https://getbootstrap.com/docs/4.1/components/modal/#via-data-attributes). There are two things to note. First they use the same data attribute for all instances even if there are multiple modals on the page. Secondly they do not apply an attribute to the modal form. They have two attributes on the toggle button, data-toggle defines the action while data-target tells the button what to show as a modal.

So a solution might look like:

<form>
    <input id="password1" type="password" autocomplete="current-password" name="password" placeholder="Enter Your Password">
    <a data-js="revealPassword" data-target="#password1">Show Password</a>

    <input id="password2" type="password" autocomplete="current-password" name="password" placeholder="Enter Another Password">
    <a data-js="passwordReveal" data-target="#password2">Show     
</form>

<script>
  document.addEventListener("DOMContentLoaded", function(event) {
    var passwordReveals = document.querySelectorAll("[data-js=passwordReveal]");
    Array.prototype.forEach.call(passwordReveals, function (item) {
      var targetSelector = item.getAttribute('data-target');
      if (!targetSelector)
        return;
      var target = document.querySelector(targetSelector);
      if (!target)
        return;

      item.addEventListener("click", function (){
        if (target.type === "password") {
            target.type = "text";
        } else {
            target.type = "password";
        }
      });
    });
  });
</script>
\$\endgroup\$
  • \$\begingroup\$ Hey Marc. Thanks for answering my answer quickly, I understand all of what you have suggested I do. I had most of the steps written out I just couldn't figure out how to add it However, I can't seem to rap my head around the Array.prototype it's awfully confusing \$\endgroup\$ – Alex Machin Aug 28 '18 at 12:38
  • \$\begingroup\$ Yes, I agree it is confusing. Basically an array has a really handy forEach method so you could normally call someArray.forEach(...). However querySelectorAll returns a NodeList which doesn't have that method. Basically what that code does is take the forEach method from an array and call it with the list of nodes returned by querySelectorAll. \$\endgroup\$ – Marc Rohloff Aug 28 '18 at 16:06
  • \$\begingroup\$ Side note, I've seen examples that just use passwordReveals.forEach so maybe that's not 100% correct. You could test it that way. \$\endgroup\$ – Marc Rohloff Aug 28 '18 at 16:07

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.