Issue/6851 add epilogue social by theck13 · Pull Request #7048 · wordpress-mobile/WordPress-Android

theck13 · 2017-12-28T01:36:07Z

Fix

Add an epilogue screen for social signup as described in #6851.

Test

Clear app data.
Tap Sign Up button.
Tap Sign Up with Google button.
Choose Google account.
Notice Epilogue for Social screen.
Edit Display Name field.
Notice display name in header updates while editing.
Tap Continue button.
a. If display name was changed, notice progress dialog.
b. If display name was not changed, notice no progress dialog.

Note

To test this code with a wasabiDebug or wasabiRelease build, you must go through the steps to add your development environment configuration to the google-services.json file. Alternatively, you can download a vanillaDebug, vanillaRelease, zaplhaDebug, or zalphaRelease APK from Buddybuild.

…condary button

… start

…ogue

hypest

Made a first pass @theck13 and left a few comments; let me know if anything's not clear. I will do some more testing but wanted to give the chance to address feedback earlier than later. Thanks!

hypest · 2018-01-18T09:33:49Z

...ess/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueSocialFragment.java

+import javax.inject.Inject;
+
+public class SignupEpilogueSocialFragment extends LoginBaseFormFragment<SignupEpilogueListener> {
+    private String mEmailAddress;


Super minor: it seems that there's a convention in the rest of wpandroid's codebase to have the constants defined at the top of the module file and have the instance data definitions after. What do you think about shuffling this around to match the convention?

Doing a quick search of the code shows examples of constant variables both before and after instance variables. Let's leave it as is for now and address things like this when implement code style rules (e.g. Checkstyle) that way we can enforce a single convention.

hypest · 2018-01-18T09:36:47Z

...ess/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueSocialFragment.java

+
+    public static final String TAG = "signup_epilogue_fragment_tag";
+
+    @Inject


Another super minor: thanks to FluxC conventions, we seem to put the @Inject annotation on the same line as the field definition itself. What do you think about matching that? Thanks!

I updated the annotation in f37ba70.

hypest · 2018-01-18T09:45:38Z

...ess/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueSocialFragment.java

+
+    @Override
+    protected @LayoutRes int getContentLayout() {
+        return 0;


Let's add some comment here to explain the 0 (perhaps similar to this comment), thanks!

I add the comment in 0ab9db4.

hypest · 2018-01-18T09:46:17Z

...ess/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueSocialFragment.java

+
+    @Override
+    protected void setupLabel(@NonNull TextView label) {
+    }


Similar to above, let's add some comment on the reason why the method is empty, thanks!

I added a comment in c3f9ca2.

hypest · 2018-01-18T10:03:11Z

WordPress/src/main/res/layout/signup_epilogue.xml

+                    </org.wordpress.android.widgets.WPTextView>
+
+                    <org.wordpress.android.widgets.WPTextView
+                        android:id="@+id/signup_epilogue_header_username"


Judging from the code, we seem to only put email addresses here so, let's rename this accordingly to avoid confusion. WDYT?

I updated the identification in be4851b.

hypest · 2018-01-18T10:28:15Z

WordPress/src/main/res/layout/signup_epilogue.xml

+
+            </android.support.v7.widget.CardView>
+
+            <org.wordpress.android.widgets.WPLoginInputRow


With WPLoginInputRow moved to the loginlib, this now needs to be updated to org.wordpress.android.login.widgets.WPLoginInputRow. Same for the second input box below.

I updated the views in ce7429a.

hypest · 2018-01-18T11:50:23Z

...ess/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueSocialFragment.java

+        mInputUsername.getEditText().setOnKeyListener(new View.OnKeyListener() {
+            @Override
+            public boolean onKey(View view, int keyCode, KeyEvent event) {
+                // Consume keyboard events except for Enter (i.e. click/tap) and Tab (i.e. focus/navigation).


Was reading the comment (and understood the code) but wasn't aware why we need to do that. Then I figured it out: it's because we basically hack the input box. We want it to look but not behave like one and instead launch the dedicated username changer UI. What do you think about adding that reasoning to the comment here? Thanks!

I added a comment in 6caef61.

hypest · 2018-01-18T14:26:32Z

...ess/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueSocialFragment.java

+        DialogInterface.OnClickListener dialogListener = new DialogInterface.OnClickListener() {
+            @Override
+            public void onClick(DialogInterface dialog, int which) {
+                switch (which) {


What do you think about adding the DialogInterface.BUTTON_NEUTRAL case too, with a comment that "nothing special, just let the dialog dimiss" since we are using that third option anyway with setNeutralButton?

That seems subjective since it doesn't make the switch statement more readable. If the case is excluded, it is ignored. If the case is added with only a comment, it is ignored. It's effectively the same. Looking at the statement now, it can be seen which cases produce actions simplifying readability. Adding a case just for a comment seems too verbose.

Cool, let's just add the comment, quickly explaining that the neutral case is ignored as the dialog is being let to simply dismiss. This makes it clear that it's a deliberate functionality, instead of overlooked case. Thanks!

I added a comment in 41a48a7.

hypest · 2018-01-18T15:57:11Z

...ess/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueSocialFragment.java

+        }
+    }
+
+    private class DownloadAvatarAndUploadGravatarTask extends AsyncTask<String, Void, Void> {


Since there is no feedback returned to the UI, can we demote the AsyncTask to a simple Thread perhaps?

I updated the class to a Thread in 12ccfd1.

…agment

…gment

hypest · 2018-01-19T11:36:44Z

Tested out the feature and works nicely @theck13 !

The only pending comment is this one.

One thing I want to run by you is: the Gravatar uploader works in parallel with the epilogue screen and I wonder what would happen if its completion happens after the user has continued/dismissed the epilogue or it fails altogether. There will be a "broken window" there but, we don't need to cover that here in this PR. What do you think about adding tracks to the error cases so we could have an idea how often the errors happen?

Come to think of it, let's hold off adding tracks to this path until we have some indication that we need the data/statistics there.

…fragment

hypest · 2018-01-19T15:37:30Z

🚢 it!

theck13 added 28 commits December 15, 2017 11:41

Add social signup epilogue screen

5ceb9a7

Add social signup flow from Google signup

8f45c1f

Add null-check to login base fragment for extended classes without se…

8e04ae9

…condary button

Update hard-coded shadow view value with resource reference

6f702c0

Update signup finished method in Google listener interface with username

6e547a2

Add analytics for completed social signup

dce261c

Add social epilogue activity

a27ec40

Add social epilogue listener interface

16462c0

Add social epilogue activity argument to main activity

cd2520e

Add showing main activity and social epilogue to activity launcher

8aba926

Update login activity actions when Google signup finished

794b163

Add showing social epilogue condition to main activity

452f7ff

Add condition to social epilogue when display name is changed

8110146

Add condition to social epilogue when username is changed

045b70f

Add condition to social epilogue when nothing is changed

5c48e7a

Add signup epilogue listener check to social epilogue on attach

43e972e

Update progress text for social epilogue

747cea7

Add attributes and request focus to view to avoid showing keyboard on…

9e09557

… start

Add showing dialog when error occurs updating account

cb15704

Add analytics tracking for social epilogue

7ecac2e

Update error dialog with retry and revert options on social epilogue

7eabc5a

Update social epilogue on account change to avoid continuing on start

f28eecb

Add ending progress to show dialog when error occurs on social epilogue

0db528d

Add task to download Google avatar and upload Gravatar on social epil…

294a69a

…ogue

Update username FluxC call with action type for social epilogue

60efe3f

Add error drawable to avatar image on signup epilogue screen

de80336

Update FluxC dependency hash to bb6ac01

85e1164

Update magic link request email action parameters

a7182ab

theck13 added Signup [Type] Enhancement labels Dec 28, 2017

Merge branch 'feature/new-signup' into issue/6851-add-epilogue-social

19b616e

aerych mentioned this pull request Dec 28, 2017

Signup: Master Issue #6838

Closed

10 tasks

theck13 added 2 commits January 17, 2018 15:22

Merge branch 'feature/new-signup' into issue/6851-add-epilogue-social

2a015db

Update signup epilogue social fragment imports for login library

6150f41

hypest suggested changes Jan 18, 2018

View reviewed changes

theck13 added 7 commits January 18, 2018 19:34

Update annotation to inline with variable

f37ba70

Add comment explaining no content layout in signup epilogue social fr…

0ab9db4

…agment

Add comment explaining no label in signup epilogue social fragment

c3f9ca2

Add comment explaining key interception in signup epilogue social fra…

6caef61

…gment

Update header email view identification in signup epilogue social fra…

be4851b

…gment

Update signup epilogue social fragment views for login library

ce7429a

Update download avatar upload gravatar async task to thread

12ccfd1

hypest approved these changes Jan 19, 2018

View reviewed changes

Add comment for neutral dialog button case in signup epilogue social …

41a48a7

…fragment

hypest merged commit 6b8e4e5 into feature/new-signup Jan 19, 2018

hypest deleted the issue/6851-add-epilogue-social branch January 19, 2018 15:37


		public static final String TAG = "signup_epilogue_fragment_tag";

		@Inject


		</android.support.v7.widget.CardView>

		<org.wordpress.android.widgets.WPLoginInputRow

Conversation

theck13 commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Test

Note

Uh oh!

hypest 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!

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!

hypest Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hypest commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hypest commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

theck13 commented Dec 28, 2017 •

edited

Loading

hypest Jan 19, 2018 •

edited

Loading

hypest commented Jan 19, 2018 •

edited

Loading