Skip to content
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.

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

Login Page Refactor #1054

Merged
merged 15 commits into from
Nov 27, 2023
Merged

Login Page Refactor #1054

merged 15 commits into from
Nov 27, 2023

Conversation

alantao912
Copy link
Contributor

@alantao912 alantao912 commented Nov 19, 2023

Login Page Refactor

What user problem are we solving?

Linked to #1055

  • Distinctive first name and last name input boxes
  • Validates name inputs
  • Password confirmation box

What solution does this PR provide?

  • Allows user to input first and last name separately
  • Checks if first and last names only contains characters
  • Forces user to type in password identically twice to prevent misinput

Testing Methodology

Clicked through registration page several times
Functionality is not broken

Any other considerations

@alantao912 alantao912 requested a review from a team as a code owner November 19, 2023 22:45
Copy link
Contributor

sweep-ai bot commented Nov 19, 2023

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.
  • Apply: Add docstrings to all functions and file headers.

@karkir0003
Copy link
Member

@alantao912 can you attach a screen recording that walks through the various edge cases

Copy link
Member

@karkir0003 karkir0003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this PR. left some questions/comments

@karkir0003
Copy link
Member

Login Page Refactor

What user problem are we solving?

  • Distinctive first name and last name input boxes
  • Validates name inputs
  • Password confirmation box

What solution does this PR provide?

  • Allows user to input first and last name separately
  • Checks if first and last names only contains characters
  • Forces user to type in password identically twice to prevent misinput

Testing Methodology

Clicked through registration page several times
Functionality is not broken

Any other considerations

can you create a github issue and link it to the PR description so that we can track the work

@alantao912 alantao912 self-assigned this Nov 20, 2023
@alantao912
Copy link
Contributor Author

@karkir0003
Copy link
Member

@karkir0003 Demo video https://github.com/DSGT-DLP/Deep-Learning-Playground/assets/70001632/4917d351-7ec8-47fe-b7b6-a4dcc43083bc

Very good!!!

Couple questions mainly on toast errors:

  1. if I enter a first name or last name like "123" and it's invalid, we should make the toast message say something like "invalid first name/last name. please enter only letters"
  2. for the error message about "firebase auth error: user in use, we should make this message a bit cleaner on the UI side"

Copy link
Member

@karkir0003 karkir0003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @alantao912. conditionally approved from my end. let's have @dwu359 take a pass

Copy link

sonarcloud bot commented Nov 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@karkir0003 karkir0003 added the code cleanup Cleaning up the code label Nov 22, 2023
Copy link
Contributor

@dwu359 dwu359 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep looks good to me

Copy link
Member

@farisdurrani farisdurrani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! Feel free to merge this in

@karkir0003 karkir0003 linked an issue Nov 26, 2023 that may be closed by this pull request
@alantao912 alantao912 added this pull request to the merge queue Nov 27, 2023
Merged via the queue into nextjs with commit f8bbc59 Nov 27, 2023
7 checks passed
@alantao912 alantao912 deleted the login-page-beautify branch November 27, 2023 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Cleaning up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Password Confirmation and Split Name field into First + Last
4 participants