Fix ScrollView Snapshots #35
Conversation
gre#34 1.evaluated the real height when the isScrollView is true 2.upgrade README.md
There was a problem hiding this comment.
thanks for the feature. I think it can be useful.
I'm a bit concerned about one thing though: it's probably that you will reach the max height the Bitmap can take (in Android at least). By experimenting, I've seen that Android don't like to have bitmap greater than the screen size (in nb of pixels). You might have an exception and promise would fail. to be tested.
Please see my comments.
README.md
Outdated
| @@ -42,6 +42,7 @@ Returns a Promise of the image URI. | |||
| - `"base64"`: encode as base64 and returns the raw string. Use only with small images as this may result of lags (the string is sent over the bridge). *N.B. This is not a data uri, use `data-uri` instead*. | |||
| - `"data-uri"`: same as `base64` but also includes the [Data URI scheme](https://en.wikipedia.org/wiki/Data_URI_scheme) header. | |||
| - **`filename`** *(string)*: the name of the generated file if any (Android only). Defaults to `ReactNative_snapshot_image_${timestamp}`. | |||
| - **`isScrollView`** *(bool)*: View is evaluated the real height when the value is true. | |||
There was a problem hiding this comment.
I'm not sure about setting that on JS side, it seems redundant information with checking view instanceof ScrollView ? OR at least, the naming is not good ('isScrollView' can be determined without the user to tell).
I guess such a argument is still a good idea to have on JS side, because you want to choose between one of these option: snapshot the ScrollView actual display (default) vs the underlying 'container' size?
then maybe you need to rename to something like snapshotContentContainer . to reflect the 'contentContainer' naming convention of RN ScrollView. or maybe you have a better name?
There was a problem hiding this comment.
second note: best would be to also implement it in iOS, but otherwise, please note in the README it's Android only for now
There was a problem hiding this comment.
yeah, I also think that. isScrollView is not good ,but i can't think other one. i am english is very bad.
and then, i can't implement it in iOS, because i can't coding swift,. i will try iOS but not just yet
| h=0; | ||
| ScrollView scrollView = (ScrollView)view; | ||
| for (int i = 0; i < scrollView.getChildCount(); i++) { | ||
| h += scrollView.getChildAt(i).getHeight(); |
There was a problem hiding this comment.
is that the best reliable way to compute the height? there is no other available option on ScrollView? what if one of the child have margin?
There was a problem hiding this comment.
i don't know that is the bast reliable way to compute the height ; i find them by google. and i use in my project is success. but i don't tested one of the child have margin scenc. tomorrow i will be trying this;
There was a problem hiding this comment.
@gre
hi, i tested it, it can compute the realy height, when the one of the child have margin(two in de ScrollView, one marginTop 100 ,other one marginTop -300). now, how i rename isScrollView to snapshotContentContainer and modify README. create new pull request or just modify in my fork project
There was a problem hiding this comment.
yeah you can modify in your branch and push to your github fork
There was a problem hiding this comment.
it's ok. you see again
#34
1.evaluated the real height when the isScrollView is true
2.upgrade README.md