Conversation
Generated by 🚫 Danger |
|
| App Name |  WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr21760-fa7d27e | |
| Commit | fa7d27e | |
| Direct Download | wordpress-prototype-build-pr21760-fa7d27e.apk |
|
| App Name |  Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr21760-fa7d27e | |
| Commit | fa7d27e | |
| Direct Download | jetpack-prototype-build-pr21760-fa7d27e.apk |
| text = stringResource( | ||
| R.string.me_email_verification_sent_description, | ||
| emailAddress | ||
| ), |
There was a problem hiding this comment.
I noticed in the video that the full text was "Check your inbox at for the confirmation ...". Maybe the emailAddress can be an empty string? In that case, we should not use the string format ... at %s ....
There was a problem hiding this comment.
@crazytonyli Thanks for catching that, it was the result of a recent change. Fixed in 940a469.
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
The testing plan succeeded for me. I left a few suggestions to consider. Please feel free to await further feedback, if desired.
|
|
@ParaskP7 This PR has been approved so I'll go ahead and merge it to avoid it "hanging around." However, my next task (TMPFRG-631-linear-issue) involves this same code, so if you still want to look at this PR feel free to share any concerns you find and I'll address them in the next PR. |
|
👋 @nbradbury and once again thanks for the ping!
Although I understand you are just trying to get another pair of eyes on that, please don't get blocked by me on such UI related PRs. I usually won't be able to be much of help to you unless I put effort to catch-up with the ins-and-out on the requirement(s), brush up my knowledge on advanced coroutine use-cases (etc), it not being something I do as part of my Core Android role for a while now. FYI: Other product teams wouldn't ask us for such reviews and as such we haven't done that for a while, nor it is expected from us. I do understand the situation with JP/WPAndroid might be a bit different, fewer resources and all. I just wanted to set clear expectations, nothing more, nothing less. 🙏 Having said that, I did do a skim review on the ViewModel side of things. Again, could help you much, without me investing more time on it. The only suggestion for me would be to add an associated view model test suite so that to help any readers understand any or all the use cases involved without needing to have it trigger this whole flow via manual UI testing. This will also help keeping this view model class be more maintainable in the long run. 🤔 |
6 participants
This PR adds a banner to the Me screen asking users to verify their email address if they haven't already done so. Before testing, I recommend viewing the compose previews in EmailVerificationBanner to see the various states. Note that if the previews are dimmed just ignore it - this appears to be a recent Android Studio issue.
@crazytonyli I'm not expecting you to review this, I just wanted you to see the Android implementation in case you have suggestions.
@ParaskP7 And I'm not expecting you to test this, I simply thought it would be good to have another Android dev look at the code, especially the ViewModel.
android-flow.mp4
To test success:
To test failure:
UNVERIFIEDhereSome notes about this PR:
ComposeViewin the Me screen to host it.Related iOS PR