Skip to content

Issue/7166 remove legacy signup#7195

Merged
hypest merged 14 commits intofeature/new-signupfrom
issue/7166-remove-legacy-signup
Feb 7, 2018
Merged

Issue/7166 remove legacy signup#7195
hypest merged 14 commits intofeature/new-signupfrom
issue/7166-remove-legacy-signup

Conversation

@theck13
Copy link
Copy Markdown
Contributor

@theck13 theck13 commented Feb 2, 2018

Remove legacy signup code that has been superseded by the new signup flow as described in #7166.

@theck13 theck13 added the Signup label Feb 2, 2018
@theck13 theck13 requested a review from hypest February 2, 2018 17:13
@aerych aerych mentioned this pull request Feb 2, 2018
10 tasks
Copy link
Copy Markdown
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice seeing the legacy code go @theck13 , good job!

I only left a few minor comments below.

<category android:name="android.intent.category.DEFAULT" />
<category android:name="android.intent.category.BROWSABLE" />
<data
android:host="signin"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @maxme has suggested over Slack, let's check with our #helpshift friends whether they need/use this shortcut to have people logged in. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with #helpshift and it doesn't seem to be used so we can remove it.

}

public static boolean isLoginWizardStyleActivated() {
return BuildConfig.LOGIN_WIZARD_STYLE_ACTIVE || getBoolean(UndeletablePrefKey.LOGIN_WIZARD_STYLE_ACTIVE, false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove UndeletablePrefKey.LOGIN_WIZARD_STYLE_ACTIVE as well perhaps?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can. I removed the preference key in 5e8d7b3.

// no account, load new account view
Intent i = new Intent(AddQuickPressShortcutActivity.this, SignInActivity.class);
startActivityForResult(i, ADD_ACCOUNT_REQUEST);
ActivityLauncher.showSignInForResult(AddQuickPressShortcutActivity.this);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ActivityLauncher.showSignInForResult uses RequestCodes.ADD_ACCOUNT as a request code but AddQuickPressShortcutActivity.java expects ADD_ACCOUNT_REQUEST in its onActivityResult. Will this still work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Good catch! I updated the request code in 46c88c1.

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Feb 7, 2018

LGTM!

@hypest hypest merged commit 9f0c9dd into feature/new-signup Feb 7, 2018
@hypest hypest deleted the issue/7166-remove-legacy-signup branch February 7, 2018 16:34
@hypest hypest mentioned this pull request Feb 23, 2018
34 tasks
ParaskP7 added a commit that referenced this pull request Dec 5, 2022
This dependency seems that was lastly used only for the no longer
existing 'SignInFragment' and 'NewUserFragment'
(see 392dba7) fragments.

However, as part of the #7195 PR, both those fragments got removed
(see 77fc93a and
161167e).

As such, this leftover from back then can be now safely removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants