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

Feat/authentication/UI #31

Merged
merged 16 commits into from
Oct 15, 2024
Merged

Feat/authentication/UI #31

merged 16 commits into from
Oct 15, 2024

Conversation

francelu
Copy link
Contributor

Overview

This PR introduces the Sign-In and Sign-Up authentication screens.

Key Features

  • Sign-In Screen:

    • Validates email and password inputs with error messages for invalid entries.
    • Will navigate unregistered users to the Sign-Up screen.
  • Sign-Up Screen:

    • Validates email, password, and password confirmation with error feedback.
  • AuthSharedComposables.kt:

    • Shared UI components for consistent design and reduced code duplication.
  • Testing:

    • UI tests cover validation and error handling for both screens.

Next Steps

  • Review and provide feedback for merging into the main branch.

@francelu francelu linked an issue Oct 13, 2024 that may be closed by this pull request
@Harrish92 Harrish92 self-assigned this Oct 13, 2024
@charliemangano charliemangano self-assigned this Oct 14, 2024
Copy link
Contributor

@charliemangano charliemangano left a comment

Choose a reason for hiding this comment

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

Very nice UI! Conforms to the Figma, great job!

Comments

  • The composable ErrorMessage is not directly related to authentication and should be moved to another file. Maybe have com.android.periodpals.ui.components folder and put it there. This would improve general organization and make it easier for us later down the line.
  • The ErrorMessage composable tests the observable String message. I think this should be done outside of it, in SignInScreen and SignUpScreen to improve readability and modularity.

Improvements

Next time, try and do smaller PRs. The SignIn.kt and SignUp.kt could have been separated into two smaller PRs. This would make it easier to review as well as improve general team organisation.

Steps before merge

Everything listed into the "Comments" section.
Don't forget to pull from main to get the AGP version bug fixed (see #32 #33).

Copy link
Contributor

@charliemangano charliemangano left a comment

Choose a reason for hiding this comment

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

lgtm!
Thank you for implementing the changes

@Harrish92 Harrish92 self-requested a review October 14, 2024 16:28
Copy link
Contributor

@Harrish92 Harrish92 left a comment

Choose a reason for hiding this comment

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

Summary:

Great work on implementing the sign-in and sign-up screens! The code is well-structured and easy to follow. The use of Compose components and state management is effective and clean.

Suggestions:

For future improvements, consider improving the validation logic for an email address. A valid email address typically has four parts:

  • Recipient name
  • @ symbol
  • Domain name
  • Top-level domain

This will help in maintaining data integrity. Keep up the good work!

@francelu francelu merged commit 2a85093 into main Oct 15, 2024
2 checks passed
@francelu francelu deleted the feat/authentication/ui branch October 15, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement auth sign-in and sign-up screens (UI)
3 participants