-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added ErrorTextField validation #1082
Added ErrorTextField validation #1082
Conversation
import UIKit | ||
import Motion | ||
|
||
open class ErrorTextFieldValidator: NSObject { |
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.
Actually there is no need to subclass NSObject
Demo project to play: demo.zip |
Somewhat related to #1041 because of implementation changes. |
Adding #1017. |
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.
Great work!
Here are a few comments for us to discuss.
Sources/iOS/ErrorTextField.swift
Outdated
layoutSubviews() | ||
} | ||
} | ||
|
||
open var isErrorRevealed: Bool { |
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.
I think we should rename this to isErrorMessageRevealed
. This will allow the isError...
prefix to be used and helpful when searching for flags through Xcode's code completion.
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.
Actually isErrorRevealed
name was in ErrorTextField
even before my changes. I kept the name same. Also note that errorLabel.text
has convenience field var error: String?
just like detailLabel.text
can be changed by var detail: String?
. I mean that field is error
not errorMessage
. Should I still rename to isErrorMessageRevealed
?
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 is fine. It can stay. I thought we were introducing a new piece and so the format should be discussed in general. A style guide needs to be developed from the current code base, and changes we would like to make for future releases.
// | ||
// Created by Orkhan Alikhanov on 30/05/2018. | ||
// Copyright © 2018 CosmicMind, Inc. All rights reserved. | ||
// |
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 header file comment needs to have the Material license added, and the default header comment removed. You can add your name as the originator at the top in the license if you like. Everything else would need to be the same. For example:
/*
* Copyright (C) 2018, Daniel Dahan and CosmicMind, Inc. <http://cosmicmind.com>.
* All rights reserved.
*
* Original Inspiration & Author
* Copyright (c) 2018 Orkhan Alikhanov <[email protected]>
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* * Redistributions of source code must retain the above copyright notice, this
* list of conditions and the following disclaimer.
*
* * Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
*
* * Neither the name of CosmicMind nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
self.textField = textField | ||
super.init() | ||
|
||
textField.addTarget(self, action: #selector(checkIfErrorHasGone), for: .editingChanged) |
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.
Let's always move all preparation code to their own prepare function called in an instance prepare function. Material follows this setup pretty much everywhere. Mainly, it is to give clear insight into code changes. For example, if I comment out a prepare function, I know that all prepare logic is contained. Where if we scatter the logic, it could lead to undesirable behavior.
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.
By the way, should I call removeTarget
somewhere, like in deinit
? I couldn't find answer. Note that textField
is weak.
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.
No you don't need to call that. The reference to target is not long lived, so you don't need to detach from it.
case .custom(let closure): | ||
closure(textField) | ||
case .default: | ||
_ = textField.isValid() // will hide if needed |
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.
If we add @discardableResult
to isValid
we can remove the _ =
.
|
||
open func isValid(deferred: Bool) -> Bool { | ||
guard let textField = textField else { return false } | ||
if !deferred { textField.isErrorRevealed = false } |
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.
Let's change deferred
to isDeferred
. The is...
prefix is helpful when Xcode's auto completion pops up, as it will display all boolean or similar is...
flags, which is an easy way to investigate an API.
|
||
open class ErrorTextFieldValidator: NSObject { | ||
public typealias ValidationClosure = (String) -> Bool | ||
open var closures: [(code: ValidationClosure, msg: 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.
Shortening variables is okay in function bodies, but for function definitions, let's use full variable names. For example: msg: String
. It is more helpful when searching API definitions and it is more clear to the reader.
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.
You mean it should be like this?
(code: ValidationClosure, message: String)
I actually changed my mind, I think all msg
parameters in the function signatures should be in full form. What do you say?
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.
Yes that is correct, and agreed :)
@DanielDahan Regarding #1041, does setting We can add option something like |
@DanielDahan we can merge |
@OrkhanAlikhanov We will need to add comments to the function definitions and any class/instance variables. |
@OrkhanAlikhanov for the auto validation, you mean making an |
@OrkhanAlikhanov I think for |
Latest demo project. demo.zip |
First change is that, new
errorLabel
field is added toErrorTextField
. It will be shown in place ofdetailLabel
whenisErrorRevealed
is set totrue
. Obviously, it's added to let botherrorLabel
anddetailLabel
have their own style and text.Secondly,
ErrorTextFieldValidator
is added which allows to set various validation criteria and to check iftextField.text
passes them or not. On failureerrorLabel
will be shown with corresponding error message set alongside the validation criterion. Basic setup is like so:Validation will be checked on the sequence that's provided, so order is important.
Calling
usernameField.isValid()
will return boolean value indicating if validation passed. On failure corresponding error message will be shown. If developer needs to know validation status without showing show error message on fail, he can passtrue
todeferred
parameter.usernameField.isValid(deferred: true)
. Note that when a validation fails others (in the sequence) will not be checked.Custom validation closure can also be set to if provided ones does not cover developer's criteria
By default auto validation is delayed until first error message appears. After first validation failure, validation will be rechecked on every change in textField:

Auto-validation is turned off until validation fails first time, which happens when
isValid()
is called on Sign Up button is tappedHowever, we have option turn auto validation on without waiting first error to occur.
Auto-validation runs as user types
Turning auto validation off:
Setting custom auto validation which will be called on every textField change: