-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Issue/9157 site creation back pressed #9187
Conversation
…n-back-pressed # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/sitecreation/NewSiteCreationMainVM.kt
@malinajirka Thanks, looks good. The text wasn't reviewed, lets add |
How about if we simplified the copy, just slightly:
|
Thanks! The text updated in 8768cce |
@malinajirka I couldn't review this PR today due to the conflicts (the changeset looks weird right now). I thought about updating the PR myself, but I am not 100% sure if I can actually finish it on time anyway since I am afk for tomorrow. Could we do one of the following for this:
Really sorry about this. My unexpected afk yesterday messed up the timeline a little bit :( cc @kwonye if you can help in any way. |
@oguzkocer Don't worry about it, there is no reason to apologize;). I should have created the PR sooner. @kwonye If you had time and didn't mind reviewing it, it'd be great. I can address any feedback in the evening. But don't worry about it too much. |
Hey! I'm moving this one to 11.9 since 11.8 has been cut. If it has to land 11.8, please feel free to move it back, target the release branch and ping me to build a new beta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malinajirka I am done with my first pass. I have raised a few questions for you, but none of it is actually a blocker. Let me know what you think!
WordPress/src/main/java/org/wordpress/android/ui/posts/BasicFragmentDialog.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/sitecreation/NewSiteCreationActivity.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/sitecreation/NewSiteCreationMainVM.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/sitecreation/NewSiteCreationMainVM.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/sitecreation/NewSiteCreationMainVM.kt
Outdated
Show resolved
Hide resolved
Thanks @oguzkocer for the review;)! I've fixed all the issues. It's ready for another round. |
@malinajirka The second testing instructions state that the back button should be suppressed if the site preview is shown. I tested this and it's not suppressed. Is that really the behavior we want? I don't think we are suppressing it right now in the code. |
@oguzkocer The instruction was outdated, sorry my bad:(. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instruction was outdated, sorry my bad:(.
No worries, just wanted to double check. I reviewed the changes and tested the rest yesterday, so
Fixes #9157
Note: This PR should be reviewed after #9142 is merged.A warning dialog is shown when the user presses the back button while the site creation is already in progress - the flow is exited when they press the positive dialog button.
The flow is exited right away when they press the back button after the site preview was shown.
@SylvesterWilmott I haven't modified the style of the warning dialog to keep it consistent with the rest of the app. Is it ok? Btw has the text been reviewed?
To test:
Click on the back button and make sure it's suppressedClick on the back button and make sure the flow is exitedUpdate release notes:
RELEASE-NOTES.txt
.