Fix displayed provider order being the inverse of the order in which they were supplied #681
Conversation
|
Good find. I always wondered how the order they're displayed in relates to the code. :-) |
|
LGTM, @amandle any thoughts on this one? |
There was a problem hiding this comment.
@samtstern I fixed the README and also had a comment about the position of the email provider below.
| @@ -140,7 +140,8 @@ public void onClick(View view) { | |||
| } | |||
| }); | |||
| provider.setAuthenticationCallback(this); | |||
| btnHolder.addView(loginButton, 0); | |||
| // Account for the email provider which should always be at the bottom | |||
| btnHolder.addView(loginButton, btnHolder.getChildCount() - 1); | |||
There was a problem hiding this comment.
Now that I think about this, do we really want the email provider to always be at the bottom? Why aren't we using the supplied order from the dev?
There was a problem hiding this comment.
If we're going to have a deprecated version of the old behavior anyway then I agree, let's just fully respect the order they provide.
There was a problem hiding this comment.
Awesome, I'm on it! 😄
There was a problem hiding this comment.
@samtstern Getting the email provider to play nice with the other idp providers required a bit more boiler plate refactoring than I would have liked, but at least it's clean now! ✨ 😄
| @@ -127,6 +126,10 @@ private void populateIdpList(List<IdpConfig> providers) { | |||
| loginButton = getLayoutInflater() | |||
| .inflate(R.layout.idp_button_twitter, btnHolder, false); | |||
| break; | |||
| case EmailAuthProvider.PROVIDER_ID: | |||
| loginButton = getLayoutInflater() | |||
| .inflate(R.layout.provider_button_email, btnHolder, false); | |||
There was a problem hiding this comment.
This is where things would have gotten messy without the Provider refactor: I would have had to keep track of the email provider index and somehow insert it somewhere inside this for loop without it actually being part of the mProviders list. It would have been possible, but very messy.
There was a problem hiding this comment.
Could we take this refactor a bit further? Provider could define @LayoutRes int getButtonLayout() and then this switch could disappear and just become loginButton = getLayoutInflater().inflate(provider.getButtonLayout(), btnHolder, false)
| android:id="@+id/email_button" | ||
| style="@style/FirebaseUI.Button.AccountChooser.EmailButton" | ||
| android:text="@string/sign_in_with_email" | ||
| tools:ignore="UnusedIds"/> <!-- Used in tests --> |
There was a problem hiding this comment.
It took me a while to figure out that my build was failing because lint thinks these ids are unused so I moved the suppression from the lint-baseline to each button.
| @@ -127,6 +126,10 @@ private void populateIdpList(List<IdpConfig> providers) { | |||
| loginButton = getLayoutInflater() | |||
| .inflate(R.layout.idp_button_twitter, btnHolder, false); | |||
| break; | |||
| case EmailAuthProvider.PROVIDER_ID: | |||
| loginButton = getLayoutInflater() | |||
| .inflate(R.layout.provider_button_email, btnHolder, false); | |||
There was a problem hiding this comment.
Could we take this refactor a bit further? Provider could define @LayoutRes int getButtonLayout() and then this switch could disappear and just become loginButton = getLayoutInflater().inflate(provider.getButtonLayout(), btnHolder, false)
| import android.content.Context; | ||
| import android.content.Intent; | ||
|
|
||
| public interface Provider { |
There was a problem hiding this comment.
Let's add some comments to each method here
|
@samtstern Done! 😄 |
|
@SUPERCILEX I'm gonna merge this when the tests pass unless you have lingering concerns? |
|
@samtstern nope, I think it's good! |
@samtstern In both #309 and a PR I'm working on to fix #554, I've wanted to get providers in an orderly fashion with the most important provider being on top and the least at the bottom. In #309, I simply made the assumption that the order should be
Google->Facebook->Twitter->Email. However, I think it would be better to use the dev's supplied provider order, but unfortunately, the order in which providers are displayed and passed in is wonky so this PR fixes that.Addresses #586 (comment)
PS: I made this change in a backwards compatible way because I think people might be surprised if they just update FirebaseUI and suddenly their provider order is all messed up.