-
Notifications
You must be signed in to change notification settings - Fork 126
AnimatedTextInputFieldConfigurator -> added 'email' text input type & ability to change password type with or without disclosure #22
base: master
Are you sure you want to change the base?
Conversation
…ype, now support with/without disclosure
danielmartin
left a comment
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.
Thanks for your contribution! I wonder if it is easy to add a test covering this configuration. /cc @victorBaro @fillito
|
|
||
| static func generate() -> TextInput { | ||
| static func generateWithDisclosure(visible: Bool = false) -> TextInput { | ||
| let textField = AnimatedTextField() |
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 the default for visibility is false, you could call this as generateWithDisclosure() and not get a disclosure button, which is confusing.
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 agree, but this configurator is private, so i don't think it's a big problem. Whatever it was this types was helpful in my product, suddenly someone else come in handy.
|
Hello @morfey911 |
|
Hello @victorBaro Thanks |
| case standard | ||
| case password | ||
| case passwordWithDisclosure | ||
| case passwordWithoutDisclosure |
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 like to have the passwordWithoutDisclosure case, but leave password as it was so we do not modify existing api
| case password | ||
| case passwordWithDisclosure | ||
| case passwordWithoutDisclosure | ||
| case email |
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.
Update your branch, there is an email case already in 0.4
|
Hello @surajHT Now that I have reviewed your code, it makes sense your suggestion of removing the clear button in TextInputs. |
|
@surajHT any updates on this? |
No description provided.