-
Notifications
You must be signed in to change notification settings - Fork 108
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
Auto close on success event behaviour and customization #370
Conversation
I spoke to Victor regarding default behaviours and real life necessity of options, we both agreed on. Current Behaviour:
PR Behaviour
UPDATE: |
One Ux issue is the lack of time to inform the user after a successful Signup / ResetPassword as the closure is instant. This is not in the PR but I tried adding a simple delay to event dismiss. Which I think looks okay, it's enough time to see it. |
Debated the consistently of disabling auto close on Login, started adding it then it seemed weird, even more so for other connection types. If really wanted it can be done of course, just doesn't feel right. |
@@ -35,6 +35,7 @@ public protocol Options { | |||
var scope: String { get } | |||
var parameters: [String: Any] { get } | |||
var allow: DatabaseMode { get } | |||
var autoClose: DatabaseMode { get } |
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.
Why not a boolean I expect Lock to close on action?. This makes trickier figure out how to make lock close itself since I need to figure out what modes I want it to close. Also if I allow all actions and I set autoclose for signup I will close lock before the login? or for reset password ?
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 would certainly be simpler if it was just a flag, I was aiming for flexibility but agree sometimes we have too much and it can possibly be confusing. If we made this a Bool it would mean, only auto close .Signup or .Reset if there is no .Login.
In your scenario, by default all allow modes are set and all autoClose modes are set. Auto Close will only close .Signup or .ResetPassword if there is no .Login allowed.
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.
Possibly should be autoCloseSingleScreens: Bool or a smaller name ;)
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 use autoclose
and it's meant to close lock if the user logs in or any other action if it's the only one allowed. For consistency I'd avoid closing on auth if autoclose is false
and show the same banner message we do for the rest.
@@ -28,24 +28,26 @@ struct ObserverStore: Dispatcher { | |||
var onFailure: (Error) -> () = { _ in } | |||
var onCancel: () -> () = { } | |||
var onSignUp: (String, [String: Any]) -> () = { _ in } | |||
var onForgetPassword: (String) -> () = { _ in } |
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.
Should it be onForgotPassword
?
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.
Yeah, typo.
|
||
weak var controller: UIViewController? | ||
|
||
func dispatch(result: Result) { | ||
func dispatch(result: Result, dismissLock: 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 don't by the extra parameter. This object should be able to decide for itself if it should dismiss or not using the options
23a8946
to
267103a
Compare
I'll squash these commits after review. |
let connections: Connections | ||
let emailValidator: InputValidator = EmailValidator() | ||
|
||
init(connections: Connections, authentication: Authentication, user: DatabaseUser) { | ||
init(connections: Connections, authentication: Authentication, user: DatabaseUser, options: Options, dispatcher: Dispatcher) { |
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.
We need Options for?
|
||
weak var controller: UIViewController? | ||
|
||
init(options: Options = LockOptions()) { |
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.
Do we really need to pass all the options?
@@ -28,24 +28,37 @@ struct ObserverStore: Dispatcher { | |||
var onFailure: (Error) -> () = { _ in } | |||
var onCancel: () -> () = { } | |||
var onSignUp: (String, [String: Any]) -> () = { _ in } | |||
var onForgotPassword: (String) -> () = { _ in } | |||
var options: Options |
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 have this mutable can't we just have the flag to tell lock to close itself after an action?
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 need to know two things, Options.autoClose
and are we in a single action mode. Currently determined with Options.allow
.
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.
We can either pass them independently of Options like 2 attributes or call a method to set them from Options. My main issue is passing it in the init since it provides no value let's just default to a sensible default and allow mutating the observerstore
func dispatch(result: Result) { | ||
let dismissLock: 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 don't see the need of this flag, we are making the decision of dismissing twice, one in the case and another after the switch. It's better to do it in the case and leave the closure to execute whether it has the dismiss or just calls the callback.
@@ -35,6 +35,7 @@ public protocol Options { | |||
var scope: String { get } | |||
var parameters: [String: Any] { get } | |||
var allow: DatabaseMode { get } | |||
var autoClose: DatabaseMode { get } |
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 use autoclose
and it's meant to close lock if the user logs in or any other action if it's the only one allowed. For consistency I'd avoid closing on auth if autoclose is false
and show the same banner message we do for the rest.
Dismiss Lock on .ResetPassword when .Login not present Added ForgetPassword event handling to Dispatcher Added Dispatch/Dismiss tests to Database Password Interactor
Remove success banner display when auto closing Added autoClose to options for specifying mode auto closing Added validation for 'impossible to close Lock' miss configurations Added more dispatch tests in DatabaseInteractor and DatabasePasswordInteractor Login will always auto close for all connection types
Updated Tests
Changed autoClose flag to Bool Updated Tests Removed Options from DatabasePasswordInteractor Login now follows autoClose rules Login success message added DatabaseInteractor tests Added Dispatcher related tests unified into ObserverStore tests
267103a
to
8c75bce
Compare
Dismiss Lock on .Signup when .Login not present
Dismiss Lock on .ResetPassword when .Login not present
Added ForgetPassword event handling to Dispatcher
Added Dispatch/Dismiss tests to Database Password Interactor