Proposal: Use comments for instructions#35
Conversation
…tOfBoundsException. Deal with password wrongly clear up
Yuki-YuXin
changed the title
| handleSignUpError(actionResult) | ||
| } | ||
| is SignUpResult.AttributesRequired -> { | ||
| // AttributesRequired only happens after CodeRequired. Please refer to the SignUpCodeFragment for more information. |
There was a problem hiding this comment.
Why are we adding this comment? What are we trying to clarify?
There was a problem hiding this comment.
Giving developers some hint that why SignUpResult.AttributesRequired is an unexpected_sdk_result in this case. Currently there is no place like documentation teach developers about this.
| } | ||
| is SignUpResult.AttributesRequired, | ||
| is SignUpResult.PasswordRequired -> { | ||
| // AttributesRequired and PasswordRequired only happens after CodeRequired. Please refer to the SignUpCodeFragment for more information. |
There was a problem hiding this comment.
This block is for Password required and Attributes required result. I don't think this comment applies
There was a problem hiding this comment.
According to my experience of doing the release test cases, AttributesRequired and PasswordRequired only happens after CodeRequired. Thus, they are unexpected_sdk_result
| handleSignInError(actionResult) | ||
| } | ||
| else -> { | ||
| // Please refer to other sign in fragments for the handling of other possible results. |
There was a problem hiding this comment.
I'd just say:
| // Please refer to other sign in fragments for the handling of other possible results. | |
| // Unexpected result |
As you did in other else sections
There was a problem hiding this comment.
The comment here guide developer the route and instructions when they meet unexpected_sdk_result
| ) | ||
| } else { | ||
| displayDialog(getString(R.string.unexpected_sdk_result_title), actionResult.toString()) | ||
| // Please refer to other sign in fragments for the handling of other possible results. |
There was a problem hiding this comment.
Same as others "else" block:
| // Please refer to other sign in fragments for the handling of other possible results. | |
| // Unexpected result |
There was a problem hiding this comment.
The comment here guide developer the route and instructions when they meet unexpected_sdk_result
| displayDialog(getString(R.string.unexpected_sdk_result_title), actionResult.toString()) | ||
| } | ||
| is SignUpResult.PasswordRequired -> { | ||
| // Please double-check the user flow settings of the application (client id) to make sure it's email + otp. |
There was a problem hiding this comment.
Similar comment applies here too:
| // Please double-check the user flow settings of the application (client id) to make sure it's email + otp. | |
| // Please refer to the EmailPasswordSignInSignUpFragment for handling sign up with password. |
There was a problem hiding this comment.
I would insist on the original one because "Please refer to the EmailPasswordSignInSignUpFragment for handling sign up with password." cannot help developers to understand why SignUpResult.PasswordRequired is unexpected_sdk_result in this case.
| } | ||
| is SignUpResult.AttributesRequired, | ||
| is SignUpResult.AttributesRequired -> { | ||
| // Please double-check the user flow settings to ensure that the user flow is not set to collect the required attributes. |
There was a problem hiding this comment.
I suggest using the same comment structure here:
| // Please double-check the user flow settings to ensure that the user flow is not set to collect the required attributes. | |
| // Please refer to the EmailAttributesSignUpFragment for handling sign up with attributes. |
There was a problem hiding this comment.
The two mean different things.
In this case, we don't include the scenario of collecting attributes in different UI screens. That's the reason why SignUpResult.AttributesRequired is unexpected_sdk_result. It could be valid.
EmailAttributesSignUpFragment is for collecting things in the same UI screens.
…sampleapp/AccessApiFragment.kt Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>
…sampleapp/EmailPasswordSignInSignUpFragment.kt Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>
…sampleapp/EmailSignInSignUpFragment.kt Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>
…sampleapp/EmailSignInSignUpFragment.kt Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>
|
Imo this PR should only handle: |
| ) | ||
| } | ||
| is SignInResult.PasswordRequired -> { | ||
| // Please double-check the type(email+password/email+otp) of email you provided. |
There was a problem hiding this comment.
nit: Can this be properly formatted? Same spacing.
3 participants
Propose to use comment to provide extra instructions for developers on how to deal with unexpected SDK results.