Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unified Login & Sign-Up: login with site address from the Prologue screen #11780

Merged
merged 8 commits into from
May 12, 2020
Merged

Unified Login & Sign-Up: login with site address from the Prologue screen #11780

merged 8 commits into from
May 12, 2020

Conversation

renanferrari
Copy link
Member

Fixes #11736

To test

Instructions

  • Make sure the UNIFIED_LOGIN_AVAILABLE flag is set to true.

Scenarios

Site Address From Prologue

  1. Clear app data.
  2. Open the app and notice the Prologue Screen.
  3. Notice how the top button label is Continue with WordPress.com.
  4. Notice how the bottom button label is Enter your site address.
  5. Notice how the top button has the primary style.
  6. Tap Enter your site address.
  7. Notice the Site Address Screen.
  8. Complete the login flow normally.

Email Screen Without Site Option

  1. Clear app data.
  2. On the Prologue Screen, tap Continue with WordPress.com.
  3. On the Email Screen, notice how the option to login with a site address is not available.

Landscape Orientation

  1. Clear app data.
  2. On the Prologue Screen, rotate the device into landscape orientation.
  3. Notice how the left button label is Enter your site address.
  4. Notice how the right button label is Continue with WordPress.com.
  5. Notice how the right button has the primary style.
  6. Tap Continue with WordPress.com.
  7. On the Email Screen, notice how the option to login with a site address is not available.

Share Intent Flow

  1. Clear app data.
  2. From outside the app, try to share some media with WordPress.
  3. If the Smart Lock dialog appears, dismiss it.
  4. On the Email Screen, notice how the option to login with a site address is still available.

Notes

  • With the UNIFIED_LOGIN_AVAILABLE flag set to false, everything should behave as before.
  • Once we get rid of the feature flag, we won't need to maintain both login_prologue_bottom_buttons_container_default and login_prologue_bottom_buttons_container_unified layouts.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@renanferrari renanferrari added this to the 14.9 milestone Apr 30, 2020
@peril-wordpress-mobile
Copy link

Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps!
WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history!

  1. cd WordPress-Android
  2. git checkout issue/11736-site-from-prologue
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/11780
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/11780 and open a new PR.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 30, 2020 •
edited

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 30, 2020 •
edited

You can test the changes on this Pull Request by downloading the APK here.

planarvoid
planarvoid approved these changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, it looks good and works well 👍 Consider my suggestion about naming of the buttons. I think the current state is a bit confusing

WordPress/src/main/java/org/wordpress/android/ui/accounts/login/LoginPrologueFragment.java Outdated
inflater.inflate(R.layout.login_prologue_bottom_buttons_container_unified, bottomButtonsCard);
}

MaterialButton primaryButton = view.findViewById(R.id.login_button);
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about changing the IDs of these two buttons to something more generic? I understand primary and secondary doesn't really work here since the order is switched between the two designs but maybe something about the order? first_button and second_button? It might make it a little more readable for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Since these IDs had not been updated since the creation of that screen, I thought it might have some not-so-obvious reason for that and decided not to touch them. Upon closer inspection, I couldn't find any good reason not to change them, so I'll just go with your suggestion 🙂

Thanks for reviewing this! 🙇‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe something about the order? first_button and second_button?

Oh, I just realized that this could make things look weird on the landscape layout, since the order kind of changes there as well (the top button becomes the right button and the bottom button becomes the left one), meaning we would have the first_button on the right and the second_button on the left. This might still be a better alternative though. Let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

After talking to @mattmiklic about this, we reached the conclusion that it's fine to have the natural order of the buttons on landscape for the new layouts, instead of having them reversed. So I'm renaming the buttons to what you've sugggested and updating the landscape layout for the new layouts, but I'm keeping the old ones as they were. That means we will still have one layout where the second_button is coming first, but that will be solved sooner than later, when we get rid of the feature flag and stop maintaining the old flow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a visual before and after:

Before

Portrait Landscape
old-portrait old-landscape

After

Portrait Landscape
new-portrait new-landscape

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me!

@renanferrari renanferrari mentioned this pull request May 9, 2020
Merged
3 tasks
@planarvoid planarvoid merged commit 8c1a255 into feature/unified-login-signup May 12, 2020
@renanferrari renanferrari mentioned this pull request May 15, 2020
Closed
@renanferrari renanferrari mentioned this pull request Jul 24, 2020
Merged
@renanferrari renanferrari deleted the issue/11736-site-from-prologue branch September 4, 2020 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmiklic mattmiklic mattmiklic left review comments

@planarvoid planarvoid planarvoid approved these changes

Assignees
No one assigned
Labels
[Type] Task Unified Login & Sign-Up
Projects
None yet
Milestone
14.9
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@renanferrari @planarvoid @mattmiklic