-
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
Page/Site design picker: Fixes flickering when changing the preview mode #15943
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APKs: |
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.
Thank you for looking into this @antonis! 🙇
Site Design Picker
- When testing the fix I could still repro the issue first on business theme Leven in Site Design Picker (see preview)
- Then I tried on other theme categories (Professional, Highlights, Blog) and I could only repro it on: Professional & one Blog theme. It seems to be more common with multi-column layouts and on tablet & desktop previews.
- In my tests of the fix it was always the mobile preview flickering before the tablet or desktop preview renders.
Preview
repro_preview_leven.mov |
---|
Page Layout Picker
- Seems more stable (I retested on prod version 19.1.1 after noticing this, and there it's not)
- Only one time I noticed a brief flickering preview for tablet on a multi-column layout
- The only flicker I was able to repro more than once was in the rendering of the gallery page template, which got me thinking it's a different issue:
Preview
repro_gallery_flicker.mov |
---|
I also noticed that the issue reproduces more often on my device when recording the screen.
- Tested the fix on Samsung Galaxy S8+ (Android 9)
- Tested the issue on Samsung Galaxy S8+ (Android 9) (v19.1.1)
WordPress/src/main/java/org/wordpress/android/ui/layoutpicker/LayoutPreviewFragment.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.
Sorry for toggling "🔴 request changes" on my initial review, what I meant was to request your feedback on the different approach proposed in the comment on LayoutPreviewFragment
The changes in this PR look good to me, since there's a noticeable improvement compared to how the previews worked before.
Please check if the suggested change removes the flickering entirely, as that would be very nice. It was the case when I tested on Friday, but I wasn't very thorough 🙂
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.
Thank you @antonis for testing the suggested approach and optimising it further!
I tested the latest changes and confirm it's working like a charm on my testing device! 👍
From my side this PR is ready to be merged!
ca6bf57
to
f662215
Compare
Fixes #13952
Description
Fixes flickering when changing the preview mode in the Page and Site design pickers by accounting for the rendering delay before updating the webview loading state.
To test
Site Design Picker
Page Layout Picker
Before
before.mp4
After
after.mp4
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)
Manual testing and relied on existing tests (
ModalLayoutPickerViewModelTest
,HomePagePickerViewModelTest
)What automated tests I added (or what prevented me from doing so)
The fix involves a UI improvement for which a stable test might not be possible
PR submission checklist:
RELEASE-NOTES.txt
if necessary.