Skip to content

Conversation

@russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented May 23, 2025

See notes in PR.

@russellwheatley russellwheatley changed the title chore: improve UI for password prompt chore: improve UI for password prompt & password recovery Views May 23, 2025
/// Alert Error title
/// found in:
/// PasswordRecoveryView
public var alertErrorTitle: String {
Copy link
Member Author

Choose a reason for hiding this comment

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

Used existing "Error" title for alerts/modal.

EmailAuthView()
ScrollView {
VStack {
if authService.authView == .authPicker {
Copy link
Member Author

Choose a reason for hiding this comment

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

I made the "Welcome" title only appear when on authPicker view, other wise it appeared in the others, e.g. PasswordRecoveryView.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you intend on having a title for the forgot password view later on, you may want to abstract this into an @ViewBuilder header property or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Made it a computed property: b4ea600 (#1259)

VStack(spacing: 20) {
SecureField(authService.string.passwordInputLabel, text: $password)
.textFieldStyle(.roundedBorder)
Text(authService.string.confirmPasswordInputLabel)
Copy link
Member Author

Choose a reason for hiding this comment

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

Improved the UI for PasswordPromptSheet:

import FirebaseCore
import SwiftUI

enum PasswordRecoveryResult: Identifiable {
Copy link
Member Author

Choose a reason for hiding this comment

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

Improved logic for password recovery so modal pops up telling user to check email for recovery link, or for failure, modal pops up with error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a custom type, you can initialize a Swift standard library Result from the authService.sendPasswordRecoveryEmail() function, use it in the switch statements below, and then also omit the do...catch in sendPasswordRecoveryEmail().

Copy link
Member Author

Choose a reason for hiding this comment

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

I used Result but still need an Identifiable so that we could control the sheet: 03ebaf2 (#1259)

Also - I still needed to use do/catch as Result expected a synchronous closure and the operation is async

@russellwheatley russellwheatley changed the title chore: improve UI for password prompt & password recovery Views refactor: improve UI for password prompt & password recovery Views May 23, 2025
EmailAuthView()
ScrollView {
VStack {
if authService.authView == .authPicker {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you intend on having a title for the forgot password view later on, you may want to abstract this into an @ViewBuilder header property or something.

Button(action: {
withAnimation {
switchFlow()
if authService.authenticationState == .authenticated {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do this slightly more elegantly with a

switch authService.authView {
  case .passwordRecovery:
    // ...
  case .emailLink:
    // ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Using switch statement: c013f34 (#1259)

The only thing I don't like is including the default case at the end to satisfy compiler as we're not using updatePassword View in AuthPickerView.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can put break in the switch instead of EmptyView

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2025-05-28 at 09 13 23

Doesn't seem possible. I can leave a TODO to refactor this.

.padding(.bottom, 4)

Button(action: {
Task {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be wrapped in a Task?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, removed: b738cf5 (#1259)

import FirebaseCore
import SwiftUI

enum PasswordRecoveryResult: Identifiable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a custom type, you can initialize a Swift standard library Result from the authService.sendPasswordRecoveryEmail() function, use it in the switch statements below, and then also omit the do...catch in sendPasswordRecoveryEmail().

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

LGTM

@russellwheatley russellwheatley merged commit ef70a9d into main May 28, 2025
6 checks passed
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.

2 participants