[iOS] Fixes scrollIndicatorInsets not work in old arch#46944
Conversation
facebook-github-bot
added
CLA Signed
|
|
||
| - (void)setScrollIndicatorInsets:(UIEdgeInsets)value | ||
| { | ||
| CGPoint contentOffset = _scrollView.contentOffset; |
There was a problem hiding this comment.
this line and line 1062 should not be required.
Line 1061 set the scroll indiciator insets but it does not use the contentOffset, so why are we reading and then setting it again?
There was a problem hiding this comment.
@cipolleschi I just restore the old logic,
, maybe it's because setting indicator insets may affects the contentOffset.There was a problem hiding this comment.
it should not. There should not be side effects, and even if there are, they should be contained in the UIScrollView. Can we test what happen if we remove those extra lines?
There was a problem hiding this comment.
I test it, seems not affects contentoffset, I removed it.
| RCT_EXPORT_VIEW_PROPERTY(scrollEventThrottle, NSTimeInterval) | ||
| RCT_EXPORT_VIEW_PROPERTY(zoomScale, CGFloat) | ||
| RCT_EXPORT_VIEW_PROPERTY(contentInset, UIEdgeInsets) | ||
| RCT_EXPORT_VIEW_PROPERTY(verticalScrollIndicatorInsets, UIEdgeInsets) |
There was a problem hiding this comment.
This is a breaking change. JS would not see the vertical scroll indicator anymore and JS components that use it will break. Can we:
- deprecate the
verticalScrollIndicatorInsetsprop - add the
scrollIndicatorInsetsprops - make sure that the
verticalScrollIndicatorInsetsprop uses thescrollIndicatorInsetsunder the hood - Add a deprecation warning in JS, somehow so users that are using that prop can change it?
There was a problem hiding this comment.
@cipolleschi I don't see ScrollView props have verticalScrollIndicatorInsets, actually it's changed from scrollIndicatorInsetsprops to verticalScrollIndicatorInsets in #44820 , but I don't know why..
There was a problem hiding this comment.
Yeah, we need to keep the vertical version around, because the RCT_EXPORT_VIEW_PROPERTY macro exposes the verticalScrollIndicatorInsets to JS and there might be product code that uses that prop.
If we change it to scrollIndicatorInsets, the product code will break.
There was a problem hiding this comment.
@cipolleschi I restore the verticalScrollIndicatorInsets, but actually, if product use it, it would crash because RCTScrollView not implements verticalScrollIndicatorInsets methods.
There was a problem hiding this comment.
RCTScrollViewnot implements verticalScrollIndicatorInsets methods.
How did it work before? 😱
There was a problem hiding this comment.
I couldn't find any relevant context in the pull request at #44820. It may just be an oversight. :)
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cipolleschi merged this pull request in c1178ac. |
|
This pull request was successfully merged by @zhongwuzw in c1178ac When will my fix make it into a release? | How to file a pick request? |
* Undo expo modules patch * Upgrade expo modules release * Patch facebook/react-native#46944 * Remove explicit prebuild config dep * Bump to rm duplicates
* Undo expo modules patch * Upgrade expo modules release * Patch facebook/react-native#46944 * Remove explicit prebuild config dep * Bump to rm duplicates
Summary: Fixes facebook#46918 . break change comes from facebook#44820 and facebook#44789 . ## Changelog: [IOS] [FIXED] - Fixes scrollIndicatorInsets not work in old arch Pull Request resolved: facebook#46944 Test Plan: scrollIndicatorInsets should work in old arch. Reviewed By: philIip Differential Revision: D64173490 Pulled By: cipolleschi fbshipit-source-id: f7186439146d1b811c02e584fd1d94fcbb5d88a2
Summary: Fixes facebook#46918 . break change comes from facebook#44820 and facebook#44789 . ## Changelog: [IOS] [FIXED] - Fixes scrollIndicatorInsets not work in old arch Pull Request resolved: facebook#46944 Test Plan: scrollIndicatorInsets should work in old arch. Reviewed By: philIip Differential Revision: D64173490 Pulled By: cipolleschi fbshipit-source-id: f7186439146d1b811c02e584fd1d94fcbb5d88a2
4 participants
Summary:
Fixes #46918 . break change comes from #44820 and #44789 .
Changelog:
[IOS] [FIXED] - Fixes scrollIndicatorInsets not work in old arch
Test Plan:
scrollIndicatorInsets should work in old arch.