Issue/6852 add username changer by theck13 · Pull Request #7151 · wordpress-mobile/WordPress-Android

theck13 · 2018-01-23T01:57:15Z

Fix

Add a username changer for social signup as described in #6852.

Test

Clear app data.
Tap Sign Up button.
Tap Sign Up with Google button.
Choose Google account.
Notice Epilogue for Social screen.
Tap Username input field.
Notice Change Username screen with username suggestions.
Tap X action in toolbar.
Notice Username input field is unchanged.
Tap Username input field.
Tap any username from suggestions.
Notice username changes in header text.
Tap X action in toolbar.
Tap Discard action in dialog.
Notice Username input field is unchanged.
Tap Username input field.
Tap any username from suggestions.
Tap X action in toolbar.
Tap Cancel action in dialog.
Tap Save action in toolbar.
Notice Username input field has changed.
Tap Username input field.
Tap any username from suggestions except first suggestion.
Enter text to get new username suggestions.
Notice new username suggestions are displayed.
Notice selected username is first in suggestions.

… fragment

…social

…gment input field

hypest · 2018-01-23T10:30:29Z

Works nicely @theck13 , good job!

I'm a bit confused with the FullScreenDialogFragment class. Can you elaborate on what needs does it address? What functionality is it abstracting away from UsernameChangerFullScreenDialogFragment for example? Thanks!

theck13 · 2018-01-24T04:26:45Z

I'm a bit confused with the FullScreenDialogFragment class. Can you elaborate on what needs does it address?

I created the FullScreenDialogFragment class with the idea of reusability across the app for the fullscreen dialog pattern since it isn't included in the design or support libraries.

What functionality is it abstracting away from UsernameChangerFullScreenDialogFragment for example?

The biggest advantage of wrapping FullScreenDialogFragment around UsernameChangerFullScreenDialogFragment is that it allows us to use any Fragment as a fullscreen dialog rather than only DialogFragments.

If we would like to go with a different implementation, we can create an optimization issue and an associated pull request once we find a better solution.

hypest

Thanks for the explanation @theck13 !

I think that at present, FullScreenDialogFragment doesn't seem to engulf key functionality that UsernameChangerFullScreenDialogFragment would be too verbose/bloated to implement itself if we subclass a DialogFragment directly.

That said, this PR works as designed and I don't see a good reason to block it on this. So, yeah, let's open an optimization ticket to implement the full-screen-dialog directly in UsernameChangerFullScreenDialogFragment, thanks!

In the meantime I finished my review pass and left some fairly minor comments. Please have a look! Thanks!

hypest · 2018-01-24T21:21:19Z

...n/java/org/wordpress/android/ui/accounts/signup/UsernameChangerFullScreenDialogFragment.java

+    protected boolean mShouldWatchText;  // Flag handling text watcher to avoid network call on device rotation.
+    protected int mUsernameSelectedIndex;
+
+    public static final String EXTRA_DISPLAY = "EXTRA_DISPLAY";


I guess this one was meant to be named EXTRA_DISPLAY_NAME instead, right?

I updated the constant name in 7b63b43.

hypest · 2018-01-24T21:31:08Z

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


+    protected void launchDialog() {
+        final Bundle extras = new Bundle();
+        extras.putString(UsernameChangerFullScreenDialogFragment.EXTRA_DISPLAY, mEditTextDisplayName.getText().toString());


What do you think about introducing a "newInstance" type of method to hide away the setup of the input bundle? For example, a UsernameChangerFullScreenDialogFragment.newInputBundle() static method could accept the displayName and the username as parameters and return a properly populated bundle ready to be passed to .setContent() below.

I added the newBundle method in 1537644.

hypest · 2018-01-24T21:42:20Z

...n/java/org/wordpress/android/ui/accounts/signup/UsernameChangerFullScreenDialogFragment.java

+            }
+        }
+
+        mUsernamesAdapter = new UsernameChangerRecyclerViewAdapter(getActivity(), suggestionList);


Perhaps here's an opportunity to reuse setUsernameSuggestions() after setting mUsernameSelectedIndex = 0?

I updated the method in 71818d2.

hypest · 2018-01-24T21:43:16Z

...n/java/org/wordpress/android/ui/accounts/signup/UsernameChangerFullScreenDialogFragment.java

+
+    private void setUsernameSuggestions(List<String> suggestions) {
+        mUsernamesAdapter = new UsernameChangerRecyclerViewAdapter(getActivity(), suggestions);
+        mUsernamesAdapter.setOnUsernameSelectedListener(UsernameChangerFullScreenDialogFragment.this);


Qualifying the this with UsernameChangerFullScreenDialogFragment is probably redundant here, right?

It's a little verbose, but it doesn't hurt anything and it makes things absolutely clear.

hypest · 2018-01-24T21:46:09Z

...n/java/org/wordpress/android/ui/accounts/signup/UsernameChangerFullScreenDialogFragment.java

+    }
+
+    protected void showDismissDialog() {
+        new AlertDialog.Builder(getContext())


This alert dialog seems to not survive rotation. It's not super important to survive it so I'll leave it up to you to assess if this needs to be addressed. Thanks!

I updated the dialog to survive device rotation in 1f25851.

hypest · 2018-01-24T21:49:07Z

...n/java/org/wordpress/android/ui/accounts/signup/UsernameChangerFullScreenDialogFragment.java

+    public static final String KEY_USERNAME_SELECTED_INDEX = "KEY_USERNAME_SELECTED_INDEX";
+    public static final String KEY_USERNAME_SUGGESTIONS = "KEY_USERNAME_SUGGESTIONS";
+    public static final String RESULT_USERNAME = "RESULT_USERNAME";
+    public static final int GET_SUGGESTIONS_INTERVAL_MS = 2000;


Highly subjective: 2secs feel a bit much for my taste. Wdyt about making it 1s or around 1s?

This is subjective and fairly arbitrary. I chose a value that would try to ensure the user has stopped typing in order to avoid premature and frequent network calls. It would be better if we had data to support that decision though. @folletto, do you have any idea what a good time would be for this interaction? This value is the amount of time in milliseconds to wait after the user has stopped typing to fetch the username suggestions from the API.

The delay should be sub-1s. I think we use 250-300ms in search in Calypso, with debounce and sequencing the calls to avoid an old call to reset a newer call.

I'm debounching the calls based on an (arbitrarily chosen) 400ms delay for the relevant constant in site-creation domain selection screen. I can adjust it to 250-300 if we decide to.

I think we use 250-300ms in search in Calypso

I would argue that we should not follow the desktop pattern for mobile in this case and avoid making multiple network calls in quick succession for a few reasons.

Metered data

Network speed

Network reliability

Typing speed

Typing reliabiliy

Waiting 2000ms might be too slow, but 300ms feels way too fast. @hypest's suggestion of 1000ms or more might be a good compromise.

I disagree here. Calypso has all the same considerations for mobile web. That timing is also a timing based on human expectation of a response, and bounce+sort take care of the networking aspect regardless of the connection.

Native here has also the advantage of having faster response times than a web app in both typing and network stack, compared to Calypso on mobile on point 2+3+4+5.

I updated the value in ec00d88.

…agment

hypest · 2018-01-25T02:05:52Z

Thanks for the changes @theck13 ! Let me know if another pass will be needed or this is ready for merge. It's 👍 from my side. Thanks!

… fragment

hypest · 2018-01-26T22:08:44Z

The discussion in #7151 (comment) got worked on over chat so, let's get this merged @theck13 !

theck13 added 17 commits January 17, 2018 15:59

Add full screen dialog fragment class

8cfaf68

Update full screen dialog fragment view ID's

2d93b5b

Add username changer full screen dialog fragment class

454c12e

Add username changer full screen dialog to social epilogue

c6370e9

Update username changer dialog fragment for signup epilogue social

63aea35

Add single choice item layout for recycler views with radio buttons

9d1fec9

Add username changer recycler view adapter class

4965b1c

Add username changer recycler view adapter to username changer dialog…

370216b

… fragment

Add username suggestions callback to username changer dialog fragment

2cb26e0

Add username suggestions callback application logging and error tracking

ed8a97c

Update signup epilogue social layout input view for login library

fb74f82

Add progress spinner to username changer dialog fragment while loading

fc2b25f

Add hiding keyboard on dialog fragment confirm and dismiss

478532c

Update child fragment manager to fragment manager in signup epilogue …

258a2d8

…social

Update FluxC dependency hash to 286f250

62f2332

Remove unnecessary reference from username changer layout

dc34c07

Add alphanumeric character restriction to username changer dialog fra…

c42746e

…gment input field

theck13 added [Type] Enhancement Signup labels Jan 23, 2018

aerych mentioned this pull request Jan 23, 2018

Signup: Master Issue #6838

Closed

10 tasks

theck13 added 2 commits January 22, 2018 19:02

Merge branch 'feature/new-signup' into issue/6852-add-username-changer

3b2e4de

Update erroneous view reference from merge conflict resolution

7113da4

hypest self-requested a review January 23, 2018 10:23

Update header text in username changer dialog fragment

296f931

Move progress spinner over search icon in username changer layout

8a31f05

hypest suggested changes Jan 24, 2018

View reviewed changes

theck13 added 2 commits January 24, 2018 18:22

Add bundle method to username changer fullscreen dialog fragment

1537644

Update display name constant in username changer fullscreen dialog fr…

7b63b43

…agment

theck13 added 2 commits January 24, 2018 18:26

Update populating username suggestions to use set method

71818d2

Update username changer dismiss dialog to survive device rotation

1f25851

hypest approved these changes Jan 25, 2018

View reviewed changes

Update fetching suggestions interval value in username changer dialog…

ec00d88

… fragment

hypest merged commit 5b388bd into feature/new-signup Jan 26, 2018

hypest deleted the issue/6852-add-username-changer branch January 26, 2018 22:08

Conversation

theck13 commented Jan 23, 2018

Fix

Test

Uh oh!

hypest commented Jan 23, 2018

Uh oh!

theck13 commented Jan 24, 2018

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!

folletto Jan 25, 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!

hypest commented Jan 25, 2018

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

folletto Jan 25, 2018 •

edited

Loading

hypest commented Jan 26, 2018 •

edited

Loading