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.

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

Unified Login: Jetpack not connected error screens #2816

Merged
merged 20 commits into from
Sep 8, 2020

Conversation

AmandaRiu
Copy link
Contributor

@AmandaRiu AmandaRiu commented Sep 2, 2020

Closes #2657 by implementing new designs and button options on the "Jetpack not connected" screen:

Light Dark
Screenshot_1599080606 Screenshot_1599080612

To Test

  1. Tap Enter your store address
  2. Enter a store address where Jetpack is either not installed or not active/connected.
  3. Verify the Jetpack Required screen is displayed (see screenshot above)
  4. Verify store address populates in the message.
  5. Click What is Jetpack and verify the "What is jetpack" dialog appears. Close dialog.
  6. Tap Learn how to install and connect Jetpack and verify help page loaded successfully
  7. Tap to go back to Jetpack required screen.
  8. Tap Log in with another account - verify routed back to the login prologue screen

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-woocommerce
Copy link

peril-woocommerce bot commented Sep 2, 2020

Fails
🚫

Danger failed to run /app/danger-0.rtnc0skjx9m.ts.

🚫

Danger failed to run /app/danger-0.rf3xznzpvx.ts.

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 woocommerce-android
  2. git checkout issue/2657-woo-screens
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/2816
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/2816 and open a new PR.

Error TypeError

Cannot read property 'added' of null
TypeError: Cannot read property 'added' of null
    at Object.exports.default (/app/danger-0.rtnc0skjx9m.ts:16:44)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

11|         warn("The PlayStoreStrings.po file must be updated any time changes are made to release notes");
12|     }
13| 
14|     // If changes were made to the strings, make sure they follow our guidelines
15|     const modifiedStrings = modifiedFiles.filter((path: string) => path.endsWith('values/strings.xml'))
----------------------------------------------^
16| 
17|     for (let file of modifiedStrings) {        
18|         const stringDiffs = await danger.git.diffForFile(file)
19| 

Error TypeError

Cannot read property 'diff' of null
TypeError: Cannot read property 'diff' of null
    at checkCommitDiffs (/app/danger-0.rf3xznzpvx.ts:43:49)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

38|         if (git === undefined) {
39|             console.log("About to crash due to an error")
40|             console.log("File:", thisFile)
41|             console.log("Danger Object: ", danger)
42| 
---------------------------------------------------^
43|             if (danger !== undefined) {
44|                 console.log("Danger is no longer defined")
45|             }
46|             else {

Generated by 🚫 dangerJS

@peril-woocommerce
Copy link

peril-woocommerce bot commented Sep 2, 2020

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

@AmandaRiu AmandaRiu added this to In progress in Unified Login Sep 2, 2020
Not sure why this is but I'm guessing it has something to do with the fact that this string was modified to contain a variable but the other translations would still not have a variable so ktlint was throwing an error that the string was not properly formatted. Changing the name of the string variable seems to have fixed this.
@AmandaRiu AmandaRiu marked this pull request as ready for review September 3, 2020 20:23
@AmandaRiu AmandaRiu requested a review from a team September 3, 2020 20:23
@ThomazFB ThomazFB self-assigned this Sep 4, 2020
@ThomazFB
Copy link
Contributor

ThomazFB commented Sep 4, 2020

Hey @AmandaRiu, awesome work here! The code is looking good, functionality, UI and Dark Mode looks just fine too! I've just noticed one problem with the Jetpack required screen:

screencap-2020-09-04T054707 690Z

When using in the landscape mode, the main text goes to the limits of the screen and seen to not follow the same margin of the back arrow and the bottom buttons.

Also, just to confirm, I've noticed that the center image and the image for the What is Jetpack? Dialog doesn't appear in landscape mode, I suppose this is intentional for best UI fit?

screencap-2020-09-04T055332 015Z

@AmandaRiu
Copy link
Contributor Author

@ThomazFB Thanks for the review and awesome catch! I've fixed this issue in 8b6ed10

Also, just to confirm, I've noticed that the center image and the image for the What is Jetpack? Dialog doesn't appear in landscape mode, I suppose this is intentional for best UI fit?

Yup, the images are purposely left out in landscape view. 👍

Ready for another round!

@AmandaRiu AmandaRiu requested review from ThomazFB and removed request for a team September 4, 2020 20:40
@loremattei loremattei modified the milestones: 5.0 ❄️, 5.1 Sep 7, 2020
@loremattei
Copy link
Contributor

Hi there! I'm moving this to 5.1 because 5.0 has been cut. If you want this to make it to 5.0, please feel free to ping me.

@ThomazFB
Copy link
Contributor

ThomazFB commented Sep 8, 2020

Hey @AmandaRiu, thanks for that change! Everything is working just fine now! LGTM :shipit:

@ThomazFB ThomazFB merged commit 55f81da into feature/unified-login Sep 8, 2020
@ThomazFB ThomazFB deleted the issue/2657-woo-screens branch September 8, 2020 14:01
@AmandaRiu AmandaRiu moved this from In progress to Done in Unified Login Sep 10, 2020
@designsimply designsimply added feature: login Related to any part of the log in or sign in flow, or authentication. and removed Login labels May 6, 2021
ParaskP7 added a commit that referenced this pull request Aug 25, 2023
This is most probably due to creating and using a newer version of
'FluxC', the '2.43.0', which includes these test incompatible changes
from @atorresveiga:
- Handle nulls as empty string #2821
wordpress-mobile/WordPress-FluxC-Android#2821
- Include bundle rules #2816
wordpress-mobile/WordPress-FluxC-Android#2816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: login Related to any part of the log in or sign in flow, or authentication.
Projects
No open projects
Unified Login
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants