-
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
Fixes IllegalArgumentException due to uninitialised site in page template picker #15415
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
Hey @ParaskP7, @planarvoid 👋 |
You can test the changes on this Pull Request by downloading the APKs: |
👋 @antonis !
Thanks for creating the fix, I am on it! 🙏 |
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.
👋 @antonis !
I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟
I have left one suggestion (💡) for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to think about and maybe apply this suggestions. However, feel free to ignore them and merge the PR yourself.
Extra Suggestion (💡): Consider making the SiteModel site
within the SiteUtils.isBlockEditorDefaultForNewPost(...)
method explicitly @Nullable
to make it explicit that this method can accept a nullable site.
WordPress/src/main/java/org/wordpress/android/viewmodel/mlp/ModalLayoutPickerViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/test/java/org/wordpress/android/viewmodel/mlp/ModalLayoutPickerViewModelTest.kt
Show resolved
Hide resolved
👋 @antonis ! Thanks for your updates, those LGTM, I am still on the approved side, feel free to merge when CI completes. |
Fixes #15408
To test:
Regression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
Added a test case
PR submission checklist:
RELEASE-NOTES.txt
if necessary.