Issue/6849 add signup methods by theck13 · Pull Request #6914 · wordpress-mobile/WordPress-Android

theck13 · 2017-11-28T05:54:33Z

Fix

Add panel that slides up when the Sign Up button is tapped on the prologue screen which displays the signup methods as described in #6849. See the screenshots below for illustration. The first screenshots show the button text change on the prologue screen. The second screenshots show the signup panel in portrait orientation. The third screenshots show the signup panel in landscape orientation. Notice the third screenshots have a blank image for the Design screenshot since there was no design for the landscape orientation.

The landscape layout was derived from the prologue landscape layout. The style used for the Terms of Service text is the same as the label text in the login flow. The style used for the Sign Up with Email and Sign Up with Google buttons is the same as the Sign Up button on the prologue screen.

Test

Clear app data.
Notice Sign Up button text.
Tap Sign Up button.
Notice signup options.
Tap Sign Up with Email button.
Notice signup screen is shown.

Note

Please, review the copy of the following text from the screenshots above.

By choosing "Sign up," you agree to our Terms of Service.

michelleweber · 2017-11-28T06:03:07Z

No comments from me!

hypest

This works smoothly @theck13 , at least from my POV :)

I left a few comments, nothing really major. Let me know if something is not clear. Thanks!

hypest · 2017-11-28T09:01:02Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SignupBottomSheetDialog.java

+import org.wordpress.android.R;
+import org.wordpress.android.ui.WPBottomSheetDialog;
+
+public class SignupBottomSheetDialog extends WPBottomSheetDialog {


Are there perhaps any plans for expanding SignupBottomSheetDialog as the feature implementation progresses?

If not, and since SignupBottomSheetDialog is just a helper to setup and launch a WPBottomSheetDialog, I'd propose to reduce it to just a static .show(params) method. SignupBottomSheetDialog will not need to inherit from anything and there will also be no need for instance variables (like mResources). WDYT?

I'm not a fan of the instance variables like mResources either. However, having the SignupBottomSheetDialog instance variable in LoginActivity will help handle the view in the callbacks from the Google signup interface in subsequent changes (i.e. #6850).

I removed the instance variables from SignupBottomSheetDialog in a655a60.

hypest · 2017-11-28T09:03:07Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SignupBottomSheetDialog.java

+    private Resources mResources;
+    private TextView mTermsOfServiceText;
+
+    public SignupBottomSheetDialog(@NonNull final AppCompatActivity activity, @NonNull final SignupSheetListener signupSheetListener) {


Can the param be just a Context instead of AppCompatActivity? If yes, it's better we use the narrower class.

I updated the parameter in 8e35781.

hypest · 2017-11-28T09:10:59Z

WordPress/src/main/res/layout-land/signup_bottom_sheet_dialog.xml

+        android:text="@string/signup_terms_of_service_text"
+        app:fixWidowWords="true"
+        style="@style/LoginTheme.TextLabel" >
+    </android.support.v7.widget.AppCompatTextView>


Mostly curious here, is the ending tag (</android.support.v7.widget.AppCompatTextView>) recommended by a linter hint perhaps? Asking since for non-container elements like a TextView, I usually consider the ending tag just noise but I might be missing something in this case.

It's not required, but I find it easier to read and more consistent if all elements have both tags.

hypest · 2017-11-28T09:17:11Z

WordPress/src/main/res/layout-land/signup_bottom_sheet_dialog.xml

+
+    <android.support.v7.widget.AppCompatTextView
+        android:id="@+id/signup_tos"
+        android:background="?attr/selectableItemBackground"


May I suggest basing the TOS label on a proper button so to enable keyboard navigation (tabbing) and click of it? An example of that is the secondary button on the login screens.

I updated the label in 4715fe9.

hypest · 2017-11-28T09:20:21Z

WordPress/src/main/res/layout-land/signup_bottom_sheet_dialog.xml

+        android:layout_width="match_parent"
+        android:padding="@dimen/margin_extra_large"
+        android:text="@string/signup_terms_of_service_text"
+        app:fixWidowWords="true"


Hmm, is this attribute actually used here? The parent element is a AppCompatTextView and I'm not sure how the custom attribute can be picked up in this case.

Nope, it is not used. That is a copy/paste error. I removed it in 92968dc.

hypest · 2017-11-28T10:07:17Z

WordPress/src/main/res/values/strings.xml


+    <!-- Signup -->
+    <string name="sign_up">Sign Up</string>
+    <string name="signup_terms_of_service_text">By choosing \"Sign up,\" you agree to our %1$sTerms of Service%2$s.</string>


Cool trick with the format specifiers, ❤️ that!

hypest · 2017-11-28T10:07:56Z

...ytics/WordPressAnalytics/src/main/java/org/wordpress/android/analytics/AnalyticsTracker.java

+        SIGNUP_EMAIL_BUTTON_TAPPED,
+        SIGNUP_GOOGLE_BUTTON_TAPPED,
+        SIGNUP_TERMS_OF_SERVICE_TAPPED,
+        SIGNUP_DISMISSED,


👍 for adding Tracks from the get go!

Agreed. :)
Is there an easy way to get later all the Tracks pieces added in this milestone?

@folletto , since we're using a feature branch to gather the work on SignUp and SiteCreation, the final PR to merge to develop will have a quite easy way to see all the events added. We will just need to checkout the AnalyticsTracker.java file. Does that sound practical to you?

I can't evaluate properly — if it's simple to do, awesome! :D

Example (taken from when the new Login was merged into develop): https://github.com/wordpress-mobile/WordPress-Android/pull/6423/files#diff-578f7e9e6f43c37a5ca1ee823a493989

All the new Tracks added by the feature will be in a more-or-less single view and highlighted.

folletto · 2017-11-28T10:46:11Z

Design wise, excellent job.
Extra excellent for aligning some of the details to the existing codebase for consistency 👌

hypest · 2017-11-29T06:51:18Z

LGTM!

theck13 added 8 commits November 27, 2017 15:41

Add base class for bottom sheet dialog view

935489e

Add signup bottom sheet to prologue screen

922cfdb

Add signup bottom sheet peek behavior to show full view

1c81c65

Add hiding signup bottom sheet when email button is tapped

55b488f

Update signup button text

7873199

Add analytics for signup bottom sheet

f3cbfc1

Add state preservation on device rotation for signup bottom sheet

f2f8dc1

Move signup bottom sheet to signup package

bb3ad63

theck13 added Signup [Status] Needs Copy Review [Status] Needs Design Review A designer needs to sign off on the implemented design. [Type] Enhancement labels Nov 28, 2017

aerych mentioned this pull request Nov 28, 2017

Signup: Master Issue #6838

Closed

10 tasks

hypest self-assigned this Nov 28, 2017

hypest suggested changes Nov 28, 2017

View reviewed changes

folletto removed the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Nov 28, 2017

theck13 removed the [Status] Needs Copy Review label Nov 28, 2017

theck13 added 4 commits November 28, 2017 10:28

Update signup bottom sheet parameter from activity to context

8e35781

Update signup bottom sheet terms of service label from text to button

4715fe9

Remove unused attribute from signup bottom sheet label view

92968dc

Remove instance variables from signup bottom sheet class

a655a60

theck13 added the [Status] Not Ready for Review label Nov 28, 2017

Update signup bottom sheet label text

7bdd860

theck13 removed the [Status] Not Ready for Review label Nov 29, 2017

hypest approved these changes Nov 29, 2017

View reviewed changes

hypest merged commit 1540a95 into feature/new-signup Nov 29, 2017

hypest deleted the issue/6849-add-signup-methods branch November 29, 2017 06:51

Conversation

theck13 commented Nov 28, 2017

Fix

Test

Note

Uh oh!

michelleweber commented Nov 28, 2017

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 Nov 28, 2017 • 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!

folletto commented Nov 28, 2017

Uh oh!

hypest commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hypest Nov 28, 2017 •

edited

Loading