New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Moving to single password field for registration #7729

Merged

scottbarnes merged 5 commits into internetarchive:master from Ari1009:passwd

Feb 13, 2024

Contributor

Ari1009 commented Mar 25, 2023

Closes #7645

An button to show and hide password

Technical

Changes in html and a python file. Also used fontawesome eye icon

Testing

docker compose up , then redirect to sign in page.

Screenshot

Before

After

Stakeholders

Ari1009 changed the title ~~Passwd_vis~~ Passwd_visibility

Ari1009 mentioned this pull request

Error during local development setup. #7725

Closed

mekarpeles self-assigned this

mekarpeles added the Priority: 3 label

mekarpeles changed the title ~~Passwd_visibility~~ Moving to single passwd for registration

RayBB added the Needs: Review label

mekarpeles assigned jimchamp and unassigned mekarpeles

jimchamp requested changes

View reviewed changes

Collaborator

jimchamp left a comment

Thanks for submitting this @Ari1009! Sorry that this has taken so long to review.

I've left some review notes for this PR. Let me know if you're still interested in finishing this up. If not, I can make any necessary updates to this branch so that your contribution makes it into our codebase.

openlibrary/templates/account/create.html Outdated

@@ @@ -28,12 +28,16 @@ <h1>$_("Sign Up")</h1> @@
                               <span class="smaller lighter">$input.help</span>
                       </div>
                       <div class="input">
+                          <script src="https://kit.fontawesome.com/a076d05399.js"></script>

Collaborator

jimchamp Dec 5, 2023

Suggested change

<script src="https://kit.fontawesome.com/a076d05399.js"></script>

This script tag is being added to the page each time that field is called in this template. Anyway, we should save a copy of each visibility icon in our images directory.

openlibrary/templates/account/create.html Outdated

                           $:input.render()
+                          $if input.name == 'password':
+                          $:suffix

Collaborator

jimchamp Dec 5, 2023

Suggested change

      
                        $:suffix
          
                            <span class="password-visibility-toggle password-visibility-toggle--hidden"></span>

There is no suffix being passed for the password field. Just render the icon here, instead.

openlibrary/templates/account/create.html Outdated

+                          passwordInput.parentNode.appendChild(passwordVisibilityToggle);
+                      });
+                  </script>
+              </div>

Collaborator

jimchamp Dec 5, 2023

Suggested change

      
            </div>
          
            </div>

Add a trailing newline character to the end of this file.

openlibrary/templates/account/create.html Outdated

Comment on lines 81 to 88

+                  <style>
+                      #password-visibility-toggle {
+                          font-size: 14px;
+                          margin-left: 5px;
+                          cursor: pointer;
+                      }
+                  </style>

Collaborator

jimchamp Dec 5, 2023

Suggested change

      
                <style>
          
                    #password-visibility-toggle {
          
                        font-size: 14px;
          
                        margin-left: 5px;
          
                        cursor: pointer;
          
                    }
          
                </style>

We try to avoid adding inline styles whenever possible. New rules for the password field should be added to https://github.com/internetarchive/openlibrary/blob/master/static/css/components/form.olform.less

openlibrary/templates/account/create.html Outdated

Comment on lines 89 to 107

+                  <script>
+                      window.addEventListener("load", function() {
+                          var passwordInput = document.getElementById("password");
+                          var passwordVisibilityToggle = document.createElement("span");
+                          passwordVisibilityToggle.id = "password-visibility-toggle";
+                          passwordVisibilityToggle.innerHTML = '<i class="far fa-eye"></i>';
+                          passwordVisibilityToggle.addEventListener("click", function(e) {
+                              e.preventDefault();
+                              if (passwordInput.type === "password") {
+                                  passwordInput.type = "text";
+                                  passwordVisibilityToggle.innerHTML = '<i class="far fa-eye-slash"></i>';
+                              } else {
+                                  passwordInput.type = "password";
+                                  passwordVisibilityToggle.innerHTML = '<i class="far fa-eye"></i>';
+                              }
+                          });
+                          passwordInput.parentNode.appendChild(passwordVisibilityToggle);
+                      });
+                  </script>

Collaborator

jimchamp Dec 5, 2023

Suggested change

      
                <script>
          
                    window.addEventListener("load", function() {
          
                        var passwordInput = document.getElementById("password");
          
                        var passwordVisibilityToggle = document.createElement("span");
          
                        passwordVisibilityToggle.id = "password-visibility-toggle";
          
                        passwordVisibilityToggle.innerHTML = '<i class="far fa-eye"></i>';
          
                        passwordVisibilityToggle.addEventListener("click", function(e) {
          
                            e.preventDefault();
          
                            if (passwordInput.type === "password") {
          
                                passwordInput.type = "text";
          
                                passwordVisibilityToggle.innerHTML = '<i class="far fa-eye-slash"></i>';
          
                            } else {
          
                                passwordInput.type = "password";
          
                                passwordVisibilityToggle.innerHTML = '<i class="far fa-eye"></i>';
          
                            }
          
                        });
          
                        passwordInput.parentNode.appendChild(passwordVisibilityToggle);
          
                    });
          
                </script>

We try to avoid adding inline Javascript whenever possible. The click handler for the password visibility toggle affordance should probably be in this file, which contains other functions related to account creation:

https://github.com/internetarchive/openlibrary/blob/master/tests/unit/js/realtime_account_validation.test.js

jimchamp added Needs: Submitter Input and removed Needs: Review labels

jimchamp changed the title ~~Moving to single passwd for registration~~ Moving to single password field for registration

jimchamp removed the Priority: 3 label

Ari1009 and others added 4 commits

December 21, 2023 17:56


          Passwd_vis

7c6684c


          [pre-commit.ci] auto fixes from pre-commit.com hooks

71608a9

for more information, see https://pre-commit.ci


          Apply review notes

72d1a72


          Use localized strings

1a5fb39

jimchamp force-pushed the passwd branch from f8eac6b to 1a5fb39 Compare

December 22, 2023 02:50

jimchamp removed their assignment

codecov-commenter commented Dec 22, 2023 •

edited

Loading

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (5c72f35) 16.68% compared to head (7544828) 16.63%.

Files	Patch %	Lines
...nlibrary/plugins/openlibrary/js/password-toggle.js	0.00%	9 Missing and 1 partial ⚠️
openlibrary/plugins/openlibrary/js/index.js	0.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7729      +/-   ##
==========================================
- Coverage   16.68%   16.63%   -0.05%     
==========================================
  Files          88       89       +1     
  Lines        4681     4695      +14     
  Branches      835      838       +3     
==========================================
  Hits          781      781              
- Misses       3384     3396      +12     
- Partials      516      518       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jimchamp added Needs: Review and removed Needs: Submitter Input labels


          Adjust margin for password inputs

Collaborator

jimchamp commented Dec 22, 2023

Updated the branch with the suggested changes. Using "Show password" and "Hide password" in place of the eye icons, as only one icon was available in the Figma. Suspect that we'll be changing this in the near future.

Mobile view:

mekarpeles assigned jimchamp

jimchamp removed their assignment

Collaborator

jimchamp commented Jan 4, 2024 •

edited

Loading

Somebody other than me needs to review this, as I have added my own commits.

mekarpeles assigned scottbarnes

scottbarnes added On testing.openlibrary.org and removed Needs: Review labels

scottbarnes mentioned this pull request

Toggle eye button in password field of Log In form and Sign Up form #8720

Closed

scottbarnes merged commit 5098757 into internetarchive:master

4 checks passed

Achorn pushed a commit to Achorn/openlibrary that referenced this pull request


          Moving to single password field for registration (internetarchive#7729)

22c6cbd

* Use localized strings
* Adjust margin for password inputs
---------

Co-authored-by: James Champ <jameschamp@acm.org>

jimchamp removed the On testing.openlibrary.org label

Ari1009 deleted the passwd branch

May 22, 2024 12:57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet