Refactor email flow into fragments #447

samtstern · 2016-12-07T22:16:41Z

I was playing around with the new sign-in flow and the check email step felt very "jumpy" to me. You'd enter an email and could just get throw into some other screen without much chance to fix it. I think this is why the email screen was a separate step to begin with.

So I moved the email screen back to being its own step, but refactored things into Fragments so we keep all of the speed improvements implemented in the previous refactor. Now RegisterEmailActivity is a controller for a few different states, and each Fragment/Activity is doing less which I like.

Change-Id: Ibaf6e4fc280438c2d35f3d425370143d2c4e09db

samtstern · 2016-12-07T22:23:53Z

@SUPERCILEX I'd like your thoughts on this. In a way it's a step backwards since what you did had less screens, but as I played around with things more I kept getting hung up on how the register screen would just jump to something else after the email field lost focus.

samtstern · 2016-12-07T23:36:39Z

Also my tests pass locally ... can't figure out what Travis is angry about.

SUPERCILEX · 2016-12-08T00:18:43Z

@samtstern For the tests, I don't think this is related to Travis because I tried to run them on my machine and all of them failed:

For the refactoring, what if we unified email sign in into a single SignInActivity god class of sorts that deals with registering new users and welcoming back existing ones? I'm thinking of using relative layouts and View.GONE to remove the name field and insert the welcome back message if the user already exists. I'll see if I can come up with a proof of concept today so you can see what I mean.

samtstern · 2016-12-08T00:20:20Z

Would that be a fundamentally different UX flow than the fragment swap? Or just avoiding the fragments?

SUPERCILEX · 2016-12-08T00:58:05Z

I just tried your refactor, and no, it doesn't look like mine is different UX wise.

Having that extra step to register and preventing the user from changing their email once they are in the process of registering really perturbs me which is why I changed it in the first place, but I agree that sending the user to a new screen without any choice could be really frustrating.

What if we showed an Alertdialog instead of sending them directly to the welcome back activity? It could be something like "You already have an account with this email" CONTINUE, CHANGE EMAIL. What do you think of that? I feel like it's the best of both worlds.

samtstern · 2016-12-08T01:19:55Z

I'll experiment with a dialog and see how that feels

SUPERCILEX · 2016-12-08T01:24:36Z

Sounds good!

samtstern · 2016-12-08T01:24:50Z

There's also the issue of presenting users who already have an account with the "create account" screen when they attempt to sign in. My personal reaction would be to click "back" since I'd assume I was on the wrong screen for an existing user.

SUPERCILEX · 2016-12-08T01:51:43Z

Oh, that didn't occur to me. In the end, I think we should proceed with your change: it keeps the performance improvements and reduces confusion. Authentication is hard! 😄

samtstern · 2016-12-08T01:55:53Z

@SUPERCILEX yeah that didn't occur to me either when we were reviewing yours, sorry!

I'll keep playing with the UI and see if I can make the transitions nice. And then I'll figure out what's going on with the tests

Change-Id: I98c1a7b46b312738d8fc9d8ab1e5d5a14d1f7bce

samtstern · 2016-12-08T02:16:47Z

Added some nice animations:
https://drive.google.com/file/d/0B3CNQ9QwPTJ-YU41ckVGQXNpcmM/view

So now the WelcomeBack screens slide in and the RegisterEmailFragment form appears to share the email field with the CheckEmailFragment form. That adds some continuity to the whole thing.

SUPERCILEX · 2016-12-08T02:20:51Z

Oooh, that looks slick!

SUPERCILEX · 2016-12-08T02:22:16Z

auth/src/main/AndroidManifest.xml

@@ -33,7 +33,6 @@

        <activity
            android:name=".ui.email.RegisterEmailActivity"
-            android:label="@string/title_register_email_activity"


Shouldn't we replace this title with something?

It's supplied by the fragments, but I'll add a default

SUPERCILEX · 2016-12-08T02:25:47Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java


 /**
- * Activity displaying a form to create a new email/password account.
+ * Activity to control the entire email sign up flow. Plays host to {@link CheckEmailFragment}
+ * and {@link RegisterEmailFragment} and triggers {@link WelcomeBackPasswordPrompt}.


and triggers {@link WelcomeBackPasswordPrompt} and {@link WelcomeBackIdpPrompt}

SUPERCILEX · 2016-12-08T02:35:17Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailFragment.java

+        // If email is passed in, fill in the field and move down to the name field.
+        String email = getArguments().getString(ExtraConstants.EXTRA_EMAIL);
+        if (!TextUtils.isEmpty(email)) {
+            mEmailEditText.setText(email);


Don't we have to lock the email field again? We didn't have to do this before because it would check to see if the user had an existing account on every focus change. Now it looks like the user can change the email without us knowing if they already have an account. Also, shouldn't this condition always be true?

I actually think we can leave the email field unlocked. Here's basically the worst thing the user could do, and it's not so bad:

User enters [email protected] in the check email screen, gets sent to RegisterEmailFragment since it's a new account.

User then changes her mind and enters [email protected], which is not a new account.

After entering name and password, the error message says "an account already exists" (thanks to your new error handling).

In that case the user will have to press back and try again, but since that's so unlikely I think it's ok to have a sub-optimal experience for those users, and now we have the editable email field (which you mentioned that you prefer)

That's great! The user not being able to edit their email really bothered me so now that it's no longer the case... This change is going to be really cool especially with those animations! 👍

SUPERCILEX · 2016-12-08T02:40:05Z

auth/src/main/res/anim/slide_in_right.xml

+    <!--<alpha-->
+        <!--android:duration="@android:integer/config_mediumAnimTime"-->
+        <!--android:fromAlpha="0.0"-->
+        <!--android:toAlpha="1.0" />-->


Looks like these aren't being used.

SUPERCILEX · 2016-12-08T02:40:14Z

auth/src/main/res/anim/slide_out_left.xml

+    <!--<alpha-->
+        <!--android:duration="@android:integer/config_mediumAnimTime"-->
+        <!--android:fromAlpha="1.0"-->
+        <!--android:toAlpha="0.0" />-->


Same comment as above.

SUPERCILEX · 2016-12-08T02:41:50Z

auth/src/main/res/layout/register_email_layout.xml

@@ -14,23 +14,23 @@
            is showing we use a relative layout to position them absolutely
        -->
        <android.support.design.widget.TextInputLayout
-            xmlns:app="http://schemas.android.com/apk/res-auto"
            android:id="@+id/email_layout"


See comment above, we could lock the email field from here: android:enabled="false".

SUPERCILEX · 2016-12-08T04:16:21Z

auth/src/main/java/com/firebase/ui/auth/ui/email/CheckEmailFragment.java

+        if (!TextUtils.isEmpty(email)) {
+            mEmailEditText.setText(email);
+            validateAndProceed();
+        } if (mHelper.getFlowParams().smartLockEnabled) {


This should probably go on a new line.

Change-Id: I5fecad0d92466b727eea47b0ec03da876652b143

Change-Id: I0fc2e3773efff571236df288c72e250cf6f06803

samtstern · 2016-12-08T17:22:32Z

I cannot for my life replicate the issues Travis CI is having with Robolectric ...

samtstern · 2016-12-08T17:42:06Z

Related:
robolectric/robolectric#2737

googlebot · 2016-12-08T21:12:23Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

SUPERCILEX · 2016-12-08T21:34:56Z

Ok... @samtstern So it does have something to do with the manifest hack. I have a successful build here where I commented out the manifest stuff: https://travis-ci.org/SUPERCILEX/FirebaseUI-Android/builds/182411811.

samtstern · 2016-12-08T21:45:44Z

Ok that is very good to know! I will hunt based on that

samtstern · 2016-12-08T21:52:27Z

Ok undid the manifest hack ... I actually don't need it anymore. I did need it when I was trying to test the Fragments directly and I figured it couldn't hurt but apparently it can hurt haha.

SUPERCILEX · 2016-12-08T22:04:07Z

Haha yeah. 😄 It looks like you set sudo: false which gives us the OOM exception again: 57ee6c2#diff-354f30a63fb0907d4ad57269548329e3R3

Change-Id: Icb240b86747841dd6ac885ce79b31a9fcf382756

SUPERCILEX · 2016-12-08T22:13:39Z

Fixed! 😄

samtstern · 2016-12-08T22:18:44Z

@mdeck1 are you filling in for @amandle yet? If so could you PTAL at this? It looks large but it's mostly just moving logic (rather than changing it). Happy to discuss with you offline.

samtstern · 2016-12-13T22:03:12Z

@amandle PTAL. Like I said above I know it's large (in diff) so we can meet about this if you're not sure what's going on/

SUPERCILEX · 2016-12-20T03:44:46Z

@samtstern STOPSHIP!!! I was working on a some features for your refactor and noticed that screen rotation is completely broken. Rotating the screen cancels the RegisterEmailFragment and starts a the autocomplete hint. Rotating the screen a second time crashes the app because of enableAutoManage already managing the id for the autocomplete hint. I'll see if I can get in a fix in my PR.

SUPERCILEX · 2016-12-20T04:56:15Z

@samtstern I created samtstern#4 to address a slew of bugs:

samtstern · 2016-12-22T16:32:01Z

Ok I am going to merge this into the 1.1.0-dev branch so that we can continue working without so many conflicts. This change is non-controversial in spirit (since it has no API changes) so any issues with this would be quality issues. I am committed to running thorough QA before releasing 1.1.0 anyway.

If this lingers any longer it will get even more out of hand and also prevent any additional work.

SUPERCILEX · 2016-12-22T19:10:21Z

Thanks @samtstern!

Refactor email flow into fragments

18baedf

Change-Id: Ibaf6e4fc280438c2d35f3d425370143d2c4e09db

Add some better transitions/animations

0783098

Change-Id: I98c1a7b46b312738d8fc9d8ab1e5d5a14d1f7bce

SUPERCILEX reviewed Dec 8, 2016

View reviewed changes

samtstern added 2 commits December 8, 2016 08:34

Review comments and lint checks

1184b91

Change-Id: I5fecad0d92466b727eea47b0ec03da876652b143

Fix RTL lint errors

bc428ed

Change-Id: I0fc2e3773efff571236df288c72e250cf6f06803

Fix travis

57ee6c2

samtstern force-pushed the email-refactor branch from f2d7157 to 57ee6c2 Compare December 8, 2016 21:51

Move back to sudo in travis

3276b1f

Change-Id: Icb240b86747841dd6ac885ce79b31a9fcf382756

samtstern requested a review from amandle December 9, 2016 18:41

Update dependencies (#3)

18d06ae

samtstern force-pushed the email-refactor branch 2 times, most recently from e472748 to 18d06ae Compare December 20, 2016 15:57

samtstern and others added 2 commits December 20, 2016 07:57

Merge branch 'version-1.1.0-dev' into email-refactor

5b2fc22

Fix a ton of bugs (#4)

f6fb313

SUPERCILEX mentioned this pull request Dec 20, 2016

Fix broken play services checks #462

Merged

Update ExtraConstants.java (#5)

2c7ffbc

SUPERCILEX mentioned this pull request Dec 22, 2016

Cleanup + make sure IdpResponse is never null if resultCode == ResultCodes.OK #469

Merged

samtstern merged commit e074fac into firebase:version-1.1.0-dev Dec 22, 2016

SUPERCILEX mentioned this pull request Dec 23, 2016

Cleanup #470

Merged

SUPERCILEX mentioned this pull request Jan 29, 2017

Email authentication typo results in poor UX #554

Closed

Refactor email flow into fragments #447

Refactor email flow into fragments #447

Uh oh!

Conversation

samtstern commented Dec 7, 2016

Uh oh!

samtstern commented Dec 7, 2016

Uh oh!

samtstern commented Dec 7, 2016

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

googlebot commented Dec 8, 2016

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

SUPERCILEX commented Dec 8, 2016

Uh oh!

samtstern commented Dec 8, 2016

Uh oh!

samtstern commented Dec 13, 2016

Uh oh!

SUPERCILEX commented Dec 20, 2016

Uh oh!

SUPERCILEX commented Dec 20, 2016

Uh oh!

samtstern commented Dec 22, 2016

Uh oh!

SUPERCILEX commented Dec 22, 2016

Uh oh!

Uh oh!

samtstern commented Dec 8, 2016 •

edited

Loading