Remove progress dialogs by samtstern · Pull Request #1280 · firebase/FirebaseUI-Android

samtstern · 2018-05-03T00:05:05Z

Beginning investigate of #724

Video of what a Google sign in looks like:
https://drive.google.com/open?id=1LSKpQMyT1yew4A2ardO97AhoP1gbXH40

Video of full email flow (no dialogs at all!):
https://drive.google.com/open?id=1VP-fRyEut_FHlsSfDrS3aawVRZtuiAQq

General approach:

Invisible activities become visible with a dim background and a small, centered spinner
Activities with a single action button show a horizontal loading bar under the action bar and have the button disabled while loading

TODO:

Create spinner overlay with minimum show time
Pass theme to overlaid spinner
Add overlaid spinner to KickoffActivity
Add overlaid spinner to SaveCredentialsActivity
Remove dialogs from WelcomeBack screens
Remove dialogs from CheckEmailFragment
Remove dialogs from RegisterEmailFragment
Figure out loading state for AuthMethodPickerActivity

Future TODO:

Make the custom phone verification progress dialog into something else (maybe a custom button)
Remove dialogs from phone code confirmation screen (after your phone auth rewrite)

cc @SUPERCILEX wdyt of the approach?

Change-Id: Ie8e5a0c5dd0105b1b9d79ef385836286cfe6375f

Change-Id: I5b39edaa31002baa1352628b818ff46b55f28fbf

samtstern · 2018-05-03T00:56:00Z

Lint error:

  /home/travis/build/firebase/FirebaseUI-Android/auth/src/main/res/values/styles.xml:236: Error: Margin values should not be negative [NegativeMargin]
          <item name="android:layout_marginTop">-7dp</item>
                                                ^

Looks like negative margin trick is not cool. I'll have to find another way to jam the progress bar under the action bar.

Edit: I could solve it using this library
https://github.com/DreaminginCodeZH/MaterialProgressBar

But I am not sure if that's worth it:
http://www.methodscount.com/?lib=me.zhanghai.android.materialprogressbar%3Alibrary%3A1.4.2

Edit 2: went for the library. It doesn't have any deps we don't already have.

Change-Id: I95e08b3f2150387918ddf6a7455eda54a71f8296

Change-Id: I85f2cc7aa3a0e33234f7819e15fc955633175fb1

Change-Id: Iee54a95d47c0af7cc2e83cb0986153d2172007c3

Change-Id: I04e32ea437446ba4240187a3cb2d42b0a706b338

Change-Id: Ia02bdc186239fe32e804f37ac2c3c74ea715dd14

samtstern · 2018-05-04T21:03:13Z

Review note: a lot of the changes are just churn from having to indent some layouts by another level. Github's new "hide whitespace changes" setting will help a lot with that.

SUPERCILEX · 2018-05-15T03:38:04Z

Sorry for the delay, I've been bogged down writing samples for issues I submitted about I/O announcements (if only stuff worked on the first try, am I right? 🤣). Anyway, hoping to look at this and the phone auth rewrite tomorrow. If not, definitely Wednesday. 👍

SUPERCILEX · 2018-05-15T03:40:39Z

Damn, I just watched the videos... lookin' hot! 🔥

Change-Id: I6d6429742bf3d4293c611a7ac741dac2654e2ec0

SUPERCILEX

I can't say this enough: thank you! This has been nagging me months now and I just never got around to it. I have a few comments, but other than those, it's looking great so far! 😊

SUPERCILEX · 2018-05-16T02:50:44Z

auth/src/main/java/com/firebase/ui/auth/ui/FragmentBase.java

-public class FragmentBase extends Fragment {
+public class FragmentBase extends Fragment implements ProgressView {
    private HelperActivityBase mActivity;
    private ProgressDialogHolder mProgressDialogHolder;


So we haven't gotten rid of all of progress dialogs yet?

I think once you're done with Phone auth we can really kill the whole ProgressDialogHolder thing.

SUPERCILEX · 2018-05-16T02:51:23Z

auth/src/main/java/com/firebase/ui/auth/ui/InvisibleActivityBase.java

+
+import com.firebase.ui.auth.R;
+
+


Nit: extra new line

SUPERCILEX · 2018-05-16T02:53:26Z

auth/src/main/java/com/firebase/ui/auth/ui/InvisibleActivityBase.java

+        setContentView(R.layout.fui_activity_invisible);
+
+        // Create an indeterminate, circular progress bar in the app's theme
+        mProgressBar = new ProgressBar(new ContextThemeWrapper(this, getFlowParams().themeId));


Shouldn't we use the material library version to get sexiness pre-L? (And consistent behavior in general.)

SUPERCILEX · 2018-05-16T02:54:40Z

auth/src/main/java/com/firebase/ui/auth/ui/InvisibleActivityBase.java

+@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
+public class InvisibleActivityBase extends HelperActivityBase {
+
+    private static final long MIN_SPINNER_MS = 1000;


Ooof, this seems like a long time. How about 500ms or even 750?

Moved to 750. You'd be surprised how "flickery" 500 feels.

SUPERCILEX · 2018-05-16T03:04:00Z

auth/src/main/java/com/firebase/ui/auth/ui/InvisibleActivityBase.java

+    @Override
+    public void showProgress(int message) {
+        if (mProgressBar.getVisibility() == View.VISIBLE) {
+            mHandler.removeCallbacksAndMessages(null);


Glad to see you know that trick too! I've been burned so many times using removeCallbacks without the messages bit. 🤦‍♂️😆

SUPERCILEX · 2018-05-16T03:15:22Z

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

+        style="@style/FirebaseUI.TopProgressBar"
+        android:visibility="invisible"
+        app:layout_constraintTop_toTopOf="parent"
+        tools:visibility="visible"/>


Nit: space before />

SUPERCILEX · 2018-05-16T03:17:57Z

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

@@ -7,31 +7,44 @@
    android:layout_height="match_parent">

    <LinearLayout


I can't motivate you to use ConstraintLayout? 😉

I think once @lsirac finishes the ToS update I'll have to use ConstraintLayout to get around the merge conflicts anyway.

SUPERCILEX · 2018-05-16T03:19:36Z

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

+        <me.zhanghai.android.materialprogressbar.MaterialProgressBar
+            android:id="@+id/top_progress_bar"
+            style="@style/FirebaseUI.TopProgressBar"
+            android:visibility="invisible"


Should we move the visibility to the style as well?

SUPERCILEX · 2018-05-16T03:20:07Z

auth/src/main/res/values/styles.xml


    <style name="FirebaseUI.Transparent" parent="FirebaseUI">
        <item name="android:windowIsTranslucent">true</item>
-        <item name="android:windowBackground">@android:color/transparent</item>


Scared of Samsung, I see! 🤣

SUPERCILEX · 2018-05-16T03:21:31Z

auth/src/main/res/values/styles.xml

+        <item name="android:layout_width">match_parent</item>
+        <item name="android:layout_height">4dp</item>
+        <item name="android:indeterminate">true</item>
+        <item name="mpb_progressStyle">horizontal</item>


I wish the Android team would fix style attributes, those look so dang ugly! 😭 Anyway, 👍

Change-Id: I7a60cd497bc00b6b9e6f92b44df1527725d96daa

Change-Id: Id976c22c8a9a820377940a953265484bf6e1dfae

SUPERCILEX

Cool, all of your explanations make sense. 👍 Just a few more invisible thingys that could be GONE. Or would that shift the rest of the layout around?

SUPERCILEX · 2018-05-16T17:59:09Z

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

+    @Override
+    public void hideProgress() {
+       mNextButton.setEnabled(true);
+       mProgressBar.setVisibility(View.INVISIBLE);


SUPERCILEX · 2018-05-16T17:59:26Z

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

+    @Override
+    public void hideProgress() {
+        mNextButton.setEnabled(true);
+        mProgressBar.setVisibility(View.INVISIBLE);


SUPERCILEX · 2018-05-16T17:59:46Z

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

+    @Override
+    public void hideProgress() {
+        mDoneButton.setEnabled(true);
+        mProgressBar.setVisibility(View.INVISIBLE);


SUPERCILEX · 2018-05-16T18:00:05Z

auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java

+
+    @Override
+    public void hideProgress() {
+        mProgressBar.setVisibility(View.INVISIBLE);


SUPERCILEX · 2018-05-16T18:00:31Z

auth/src/main/java/com/firebase/ui/auth/ui/idp/WelcomeBackIdpPrompt.java

+    @Override
+    public void hideProgress() {
+        mDoneButton.setEnabled(true);
+        mProgressBar.setVisibility(View.INVISIBLE);


samtstern · 2018-05-16T18:37:42Z

@SUPERCILEX now that you mention it, I have gone back to all invisible/visible for the top bar as I don't want layout to jump by ~8dp each time it changes. Worth the small inefficiency IMO.

Change-Id: I65272097018400f1f9df498d757151985650d747

SUPERCILEX

SGTM!

samtstern · 2018-05-16T19:49:45Z

Gonna wait to merge this until the ToS/PP thing is done to save @lsirac from the nasty layout merge commits. Don't say I never did anything for you Leo!

Change-Id: I540fca91aa8e33f79cc364a5ed267a4fb76e6cf4

Start removing progress dialogs

2fec554

Change-Id: Ie8e5a0c5dd0105b1b9d79ef385836286cfe6375f

samtstern changed the title ~~Dialog investigate~~ [WIP] Remove progress dialogs May 3, 2018

Remove dialog from welcomeback screens

d688455

Change-Id: I5b39edaa31002baa1352628b818ff46b55f28fbf

samtstern added 2 commits May 3, 2018 11:39

Finish the check and register email flows

4b363d4

Change-Id: I95e08b3f2150387918ddf6a7455eda54a71f8296

Theme the spinner

863746e

Change-Id: I85f2cc7aa3a0e33234f7819e15fc955633175fb1

samtstern changed the title ~~[WIP] Remove progress dialogs~~ Remove progress dialogs May 3, 2018

samtstern changed the base branch from version-3.4.0-dev to version-4.0.0-dev May 4, 2018 17:24

samtstern added 2 commits May 4, 2018 10:38

Merge branch 'version-4.0.0-dev' into dialog-investigate

ee7f6dd

Change-Id: Iee54a95d47c0af7cc2e83cb0986153d2172007c3

Move to 3p material progress bar library

526529c

Change-Id: I04e32ea437446ba4240187a3cb2d42b0a706b338

samtstern requested a review from SUPERCILEX May 4, 2018 19:49

samtstern added this to the 4.0.0 milestone May 4, 2018

AuthMethodPicker

704df98

Change-Id: Ia02bdc186239fe32e804f37ac2c3c74ea715dd14

Merge branch 'version-4.0.0-dev' into dialog-investigate

a97a918

Change-Id: I6d6429742bf3d4293c611a7ac741dac2654e2ec0

SUPERCILEX reviewed May 16, 2018

View reviewed changes

samtstern added 2 commits May 16, 2018 09:29

Simple review feedback

f24d3a7

Change-Id: I7a60cd497bc00b6b9e6f92b44df1527725d96daa

Clarifying comments

23b5600

Change-Id: Id976c22c8a9a820377940a953265484bf6e1dfae

SUPERCILEX reviewed May 16, 2018

View reviewed changes

gone --> invisible

2a3dbc1

Change-Id: I65272097018400f1f9df498d757151985650d747

SUPERCILEX approved these changes May 16, 2018

View reviewed changes

Merge branch 'version-4.0.0-dev' into dialog-investigate

4d80006

Change-Id: I540fca91aa8e33f79cc364a5ed267a4fb76e6cf4

samtstern merged commit 66be717 into version-4.0.0-dev May 17, 2018

		@@ -7,31 +7,44 @@
		android:layout_height="match_parent">

		<LinearLayout


		import com.firebase.ui.auth.R;

Conversation

samtstern commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern commented May 4, 2018

Uh oh!

SUPERCILEX commented May 15, 2018

Uh oh!

SUPERCILEX commented May 15, 2018

Uh oh!

SUPERCILEX left a comment

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!

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!

SUPERCILEX left a comment

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 May 16, 2018

Uh oh!

SUPERCILEX left a comment

Choose a reason for hiding this comment

Uh oh!

samtstern commented May 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

samtstern commented May 3, 2018 •

edited

Loading

samtstern commented May 3, 2018 •

edited

Loading