Add fabric support for maintainVisibleContentPosition #32
Conversation
janicduplessis
force-pushed
the
@janic/fabric
branch
from
b2c2bd6 to
8251741
Compare
|
React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm
Outdated
Show resolved
Hide resolved
|
This is testing well with just the ScrollView on iOS, not testing well with FlatList on iOS: RPReplay_Final1665685380.MP4I wonder if I broke the VirtualizedList |
| BOOL horizontal = _scrollView.contentSize.width > self.frame.size.width; | ||
| int minIdx = props.maintainVisibleContentPosition.value().minIndexForVisible; | ||
| for (NSUInteger ii = minIdx; ii < _contentView.subviews.count; ++ii) { | ||
| // Find the first entirely visible view. This must be done after we update the content offset |
There was a problem hiding this comment.
Conceptually, shouldn't this be finding the first entirely visible view before we update the content offset?
There was a problem hiding this comment.
This comment was in the original scrollview implementation so I'm not sure what it means. I removed most of it as I think it is confusing.
| @@ -12,10 +12,33 @@ public interface UIManagerListener { | |||
| /** | |||
| * Called right before view updates are dispatched at the end of a batch. This is useful if a | |||
| * module needs to add UIBlocks to the queue before it is flushed. | |||
| * | |||
| * This is called by Paper only. | |||
There was a problem hiding this comment.
NAB for me, but I wonder if when you go to upstream they'll want you to create separate subclasses of UIManagerListener for Paper v.s. Fabric
There was a problem hiding this comment.
Or rather sub-interface ... I'm out of practice with Java but I'm pretty sure interfaces can extend other interfaces? So you'd end up having BaseUIManagerListener and PaperUIManagerListener and FabricUIManagerListener
There was a problem hiding this comment.
I think it is the same interface because this is used in an interface that abstracts UIManager and FabricUIManager. See https://github.com/facebook/react-native/blob/main/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManager.java#L110. I also agree that it is probably better as separate interfaces, but might be better to leave as is for now.
|
Confirmed that the ScrollView example is working as expected on both iOS and Android. Interestingly, the FlatList example works fine with |
janicduplessis
force-pushed
the
@janic/fabric
branch
from
39d3c8a to
3c4e77e
Compare
|
Ok it should work now. The problem was because of the loading view that the example has, it should use minIndex of 2 to avoid using that view to maintain visible content position as it will be removed. I think it works in the old arch because react doesn't batch state updates. |
Summary
Changelog
[CATEGORY] [TYPE] - Message
Test Plan