-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Upgrade guide and compat reports #744
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
Conversation
|
@SUPERCILEX did I miss anything? I feel like I did for database but I am not sure. |
docs/upgrade-to-2.0.md
Outdated
| Google/Email/Phone authentication. | ||
| * **Sign Out and Delete** - The `AuthUI#delete(Activity)` and `AuthUI#signOut(Activity)` | ||
| methods have been removed, please use the versions that accept `FragmentActivity`. | ||
| * **Error Codes** - The `ResultCodes` class has been removed, instead in `onActivityResult` you |
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.
@samtstern We actually haven't done that yet, though I'd be happy to submit a PR. (The class is here and just references the activity results).
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.
Created #745
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.
Oh interesting. We did do something about the network error code though, do you remember what that change was?
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, we used to have two error code classes, one in the UI package and the new one in the root package.
I'm not sure, but I think a long time ago we make it so that everything returned result code ok or cancelled and you would check for the idp response error code to see if it was a network error. I believe we used to return the error code directly in the result code before that.
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.
Found it and fixed.
docs/upgrade-to-2.0.md
Outdated
| * **Sign Out and Delete** - The `AuthUI#delete(Activity)` and `AuthUI#signOut(Activity)` | ||
| methods have been removed, please use the versions that accept `FragmentActivity`. | ||
| * **Error Codes** - The `ResultCodes` class has been removed, instead in `onActivityResult` you | ||
| should check for `RESULT_CANCELED` and then use `IdpResponse#getErrorCode()` to determine the \ |
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 \ looks like a typo.
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.
Done
| * **Error Codes** - The `ResultCodes` class has been removed, instead in `onActivityResult` you | ||
| should check for `RESULT_CANCELED` and then use `IdpResponse#getErrorCode()` to determine the \ | ||
| source of the error. | ||
| * **Choosing Providers** - `setProviders` has been deprecated, please use `setAvailableProviders` |
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.
Also, we removed the vargs setProviders
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.
done.
docs/upgrade-to-2.0.md
Outdated
| * **Facebook and Google Scopes** - setting scopes via `string` resources is no longer supported, | ||
| please set scopes in code using `IdpConfig`. | ||
| * **Themes** - AppCompat theme properties such as `colorPrimary` or `colorAccent` are now | ||
| respected, please double-check to make sure your UI still looks as expected. |
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 this should be rewritten to something like so:
AppCompat theme properties such as
colorPrimaryandcolorAccentare now used to style FirebaseUI automatically without any need for customization. Unless your auth UI needs a different theme than the rest of your app, please removeAuthUI.SignInIntentBuilder#setTheme(int)and its related xml theme from your auth intent builder and check to make sure that the auth UI has been themed correctly.
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.
Done.
| the saving/retrieving of full credentials from the API. Setting the same value for each flag | ||
| will emulate the previous single-flag behavior. | ||
|
|
||
| ## Database |
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.
@samtstern I think you got them all. 😄
Change-Id: I0b1ee2907f7ef1a2ea661f8fc36aeea7da1b07f5
|
@samtstern Awesome! Do we not want to go through with #745 then? I still think it's a nice improvement and v2.0 is going to be our only chance for a while. |
|
@SUPERCILEX just commented on that PR but yeah let's not go through with #745. Also while I have your attention, see my comment on your "Key doesn't exist in a ref" PR since that's the last outstanding thing. |
Issues #676 and #743 are addressed.