-
Notifications
You must be signed in to change notification settings - Fork 478
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
fix(android): 📺 makeImageFromView respect scroll offset #2306
fix(android): 📺 makeImageFromView respect scroll offset #2306
Conversation
99399ec
to
463dfd5
Compare
Also how do you format code? There is no prettier in repository so I used default VSCode formatter, but not sure if it's correct solution. |
@Nodonisko formatting is done via prettier you didn't see it because eslint takes care of it. |
this looks very cool :) |
Well done :)) I've very excited about this. |
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.
very very cool, added two small comments to the PR
<ScrollView | ||
style={styles.scrollview} | ||
ref={(ref) => { | ||
ref?.scrollTo({ y: 200 }); |
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.
@wcandillon Maybe remove these refs in all Snapshots since they doesn't work anyway because snapshot is taken before it scrolls?
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.
we do wait before taking the screenshot but it is possible that on CI it is too slow to work. Locally I was getting a consistent result. If I see that it is unstable on CI, I will indeed change 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.
You seem to be pretty sure that the test will be flaky there. I should probably be very careful there :)
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.
Yea, I would prefer slightly less cover in tests than flakines :-)
@Nodonisko thank you for this ❤️ |
Thank you too for help ❤️ |
🎉 This PR is included in version 1.0.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #2171
fixes #2063
The underlying problem is not attributed to padding but to snapshots being captured from the top part of the FlatList, which may have already unmounted some components due to scroll offset.
The update corrects a flaw where the ScrollView offset was disregarded, causing snapshots to capture only the topmost section of the ScrollView, irrespective of the user's scroll position.
In the process of fixing this, another issue surfaced: if the ScrollView doesn't extend across the full height (or width, for horizontal views), the unoccupied area would appear in the snapshot. To address this, I've implemented a clipping mechanism.
Additionally, I've added e2e tests but I probably don't know how to use them because I've encountered several issues (help appreciated):
Some screenshots for reference: