Skip to content

Improve submission form - #4

Open
ulibath wants to merge 6 commits into
mainfrom
feature/improve-email-text-field
Open

Improve submission form#4
ulibath wants to merge 6 commits into
mainfrom
feature/improve-email-text-field

Conversation

@ulibath

@ulibath ulibath commented Oct 23, 2023

Copy link
Copy Markdown
Collaborator

This PR is related to IOS-4504 and IOS-4494.

It solves the following:

  • Text fields are checked if they are empty or have spaces.
  • Email text field now has email keyboard type and email validation
  • Added email localization
  • Change submit button validation to also include email check

@ulibath
ulibath requested a review from frankschlegel October 23, 2023 12:26
Comment thread Package.swift Outdated
platforms: [
.macOS(.v12),
.iOS(.v14)
.iOS(.v15)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This might make it difficult to merge this back to the main repo.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What features require 15?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

.foregroundStyle, but I changed it to .foregroundColor now.


public var titleDescriptionCannotBeEmpty: String

public var emailAddressShouldBeValid: String

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Even thought the other properties don't have any I'd still be happy with docs. Is there any case where the email shouldn't be valid?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah this is the string that is shown if it's invalid, right? Wasn't super clear

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

.foregroundColor(textColor)
.background(fieldBackgroundColor)
.clipShape(RoundedRectangle(cornerRadius: WishKit.config.cornerRadius, style: .continuous))
.onReceive(Just(emailText)) { _ in handleEmailChange() }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

onChange makes more sense here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right. Just will also only fire once, and not when emailText changes.

Comment thread Package.swift Outdated
platforms: [
.macOS(.v12),
.iOS(.v14)
.iOS(.v15)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What features require 15?


public var titleDescriptionCannotBeEmpty: String

public var emailAddressShouldBeValid: String

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

.padding([.leading, .trailing, .bottom], 5)
}

Text(WishKit.config.localization.emailAddress + (WishKit.config.emailField == .optional ? " (optional)" : ""))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The "optional" will not be translated this way, right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It won't, but this is already the case in the main repro, and since we are not using the optional case, I haven't added this to the localization list.

.foregroundColor(textColor)
.background(fieldBackgroundColor)
.clipShape(RoundedRectangle(cornerRadius: WishKit.config.cornerRadius, style: .continuous))
.onReceive(Just(emailText)) { _ in handleEmailChange() }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right. Just will also only fire once, and not when emailText changes.

Comment thread Sources/WishKit/SwiftUI/CreateWishView.swift
guard !self.emailText.isEmpty else { return }
let emailValidationRegex = "[A-Z0-9a-z._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,64}"
let emailValidationPredicate = NSPredicate(format: "SELF MATCHES %@", emailValidationRegex)
self.isEmailValid = emailValidationPredicate.evaluate(with: self.emailText)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There seem to be better ways of validating e-mail addresses. Your call.

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.

3 participants