-
Notifications
You must be signed in to change notification settings - Fork 0
Improve submission form #4
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
base: main
Are you sure you want to change the base?
Conversation
Package.swift
Outdated
| platforms: [ | ||
| .macOS(.v12), | ||
| .iOS(.v14) | ||
| .iOS(.v15) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What features require 15?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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() } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Package.swift
Outdated
| platforms: [ | ||
| .macOS(.v12), | ||
| .iOS(.v14) | ||
| .iOS(.v15) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)" : "")) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() } |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
This PR is related to IOS-4504 and IOS-4494.
It solves the following: