-
Notifications
You must be signed in to change notification settings - Fork 110
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 mustAcceptTerms
option for Sign up
#503
Conversation
Updated README
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 see you already have the setters for customizing the privacy policy and terms URL. Where are those links accessible from? I don't see the text above the sign up button having a link color, nor the new dialog (introduced in this PR) having buttons or links to them.
@@ -244,6 +244,15 @@ class InputField: UIView, Stylable { | |||
return 0 | |||
} | |||
} | |||
|
|||
var isValid: Bool { | |||
switch self { |
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.
is this normal? switching for a boolean seems way more complicated than just storing a "valid/!valid" boolean
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.
So by default you can't do that, unless you override and add comparisons for equality operators (which is annoying) as you need to specifically add for each case.
So in this case for clarity added this.
@@ -121,6 +121,10 @@ class OptionsSpec: QuickSpec { | |||
it("should have allowShowPassword enabled") { | |||
expect(options.allowShowPassword) == true | |||
} | |||
|
|||
it("should have mustAcceptTerms disabled") { | |||
expect(options.mustAcceptTerms) == 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.
is this test testing the "default value" ?
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
@lbalmaceda you can access those from the main screen at the bottom. |
|
@lbalmaceda It may not look like it, but it is, agree it could look more obvious, it's always been like this though. |
Updated i18n Strings
Update README
Added Tests