-
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/9854 site creation loading #11578
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
# Conflicts: # WordPress/src/main/res/layout-land/site_creation_preview_screen.xml # WordPress/src/main/res/layout/site_creation_preview_screen_default.xml
Hi there! I will be freezing 14.6 in a little bit, to this PR is being bumped to 14.7. If you need this to be shipped in 14.6, please update the base branch to |
.../src/main/java/org/wordpress/android/ui/sitecreation/previews/SiteCreationPreviewFragment.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/org/wordpress/android/ui/sitecreation/previews/SiteCreationPreviewFragment.kt
Outdated
Show resolved
Hide resolved
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.
Hi @malinajirka this works beautifully! Thank you for these amazing tests! They work really well and I learnt a lot about the coroutine APIs 😉
I left some minor comments. Once those are addressed I will do a second pass to ensure that this is tested thoroughly 🙇
Thank you @jd-alexander for the review !!! ;) I've replied to your comments and simplified the code as you suggested. I also realized the drawable doesn't look as it's supposed to in the dark mode. I pinged Klym and asked him about the best approach for solving the issue. I'm adding "do not merge" tag and it might make sense to do another round of the review when I fix the issue so you can quickly test even the dark mode. Thanks! |
I've added the dark mode version of the drawable and this is now ready for test - ff51082 |
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.
Thanks @malinajirka The dark mode changes are great! Tested on both phone & tablet! All is looking well now 👍
Looks fantastic. Thanks @malinajirka! |
Fixes #9854
Goal of this PR was to improve the look of the "creating site" loading screen.
To test - test it on different screen sizes.
Anything else you can think of. This flow is vital, so should be tested with care.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.