-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
Changes fragment commit to synchronous #1066
Merged
Merged
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
3d1542a
feat: Synchronous fragment commit
1e4bd35
test: Adds issue #1017 repro
7e2f4ef
chore: merge current master
WoLewicki 021394d
fix: proper test name
WoLewicki f761068
feat: simplify transaction creation
WoLewicki bf94ef9
fix: concatenate all updates of container to avoid visual glitches
WoLewicki bebda39
chore: merge current master
WoLewicki 5ca596e
chore: merge current master
WoLewicki 9dfe15b
fix: restore commiting each transaction immediately
WoLewicki 8db979e
feat: apply suggested changes
WoLewicki de6486b
Merge branch 'master' into @ubax/fix-android-lag
WoLewicki 01b4959
feat: add experimental approach
WoLewicki 9ce6a7a
fix: add whitespace
WoLewicki 49112ec
Merge branch 'master' into @ubax/fix-android-lag
WoLewicki deadfae
feat: add comments to new code logic
WoLewicki 2c10836
feat: apply suggested renames
WoLewicki File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
import {NavigationContainer, RouteProp} from '@react-navigation/native'; | ||
import { | ||
createStackNavigator, | ||
StackNavigationProp, | ||
TransitionPresets, | ||
} from '@react-navigation/stack'; | ||
import React from 'react'; | ||
import {Alert, LogBox} from 'react-native'; | ||
import {StyleSheet, Text, View, FlatList, Pressable} from 'react-native'; | ||
import {ScrollView} from 'react-native-gesture-handler'; | ||
|
||
import { | ||
Header, | ||
Colors, | ||
DebugInstructions, | ||
} from 'react-native/Libraries/NewAppScreen'; | ||
|
||
type DataItem = { | ||
title: string; | ||
value: number; | ||
}; | ||
|
||
enum Route { | ||
FirstScreen = 'FirstScreen', | ||
SecondScreen = 'SecondScreen', | ||
} | ||
|
||
type RootStackParamList = { | ||
[Route.FirstScreen]: undefined; | ||
[Route.SecondScreen]: undefined; | ||
}; | ||
|
||
LogBox.ignoreLogs([ | ||
'VirtualizedLists should never be nested inside plain ScrollViews', | ||
]); | ||
|
||
const Stack = createStackNavigator(); | ||
|
||
const dataset = Array.from<unknown, DataItem>({length: 100}, (_, i) => ({ | ||
title: `Title ${i}`, | ||
value: i, | ||
})); | ||
|
||
const Screen: React.FC<{ | ||
route: RouteProp<RootStackParamList, any>; | ||
navigation: StackNavigationProp<RootStackParamList, any>; | ||
}> = ({route, navigation}) => { | ||
for (let i = 0; i < 10 ** 3; i++) {} | ||
const isSecondScreen = route.name === Route.SecondScreen; | ||
|
||
const handleItemPress = (item: DataItem) => () => { | ||
if (isSecondScreen) { | ||
Alert.alert(item.title, item.value.toString()); | ||
return; | ||
} | ||
navigation.navigate(Route.SecondScreen); | ||
}; | ||
|
||
const renderItem = (item: DataItem) => ( | ||
<Pressable | ||
android_ripple={{color: Colors.light}} | ||
style={styles.listItemContainer} | ||
onPress={handleItemPress(item)}> | ||
<View style={styles.listItemContentWrapper}> | ||
<Text style={styles.listItemTitle}>{item.title}</Text> | ||
<Text>{`This is item number ${item.value}`}</Text> | ||
</View> | ||
</Pressable> | ||
); | ||
|
||
return ( | ||
// So in a much more complex scenario, we might have a ScrollView that contains multiple horizontal list | ||
// I know it's bad to have a FlatList nested in ScrollView, ideally I should use something better | ||
// However, it seems like having a nested VirtualizeList in ScrollView will cause the transition lag issue | ||
<ScrollView> | ||
<FlatList | ||
data={dataset} | ||
keyExtractor={(item) => item.value.toString()} | ||
renderItem={({item}) => renderItem(item)} | ||
ItemSeparatorComponent={() => <View style={styles.separator} />} | ||
ListHeaderComponent={!isSecondScreen && Header} | ||
ListHeaderComponentStyle={styles.headerWrapper} | ||
ListFooterComponent={!isSecondScreen && DebugInstructions} | ||
ListFooterComponentStyle={styles.footerWrapper} | ||
scrollEnabled={false} | ||
/> | ||
</ScrollView> | ||
); | ||
}; | ||
|
||
const App: React.FC = () => { | ||
return ( | ||
<NavigationContainer> | ||
<Stack.Navigator screenOptions={{...TransitionPresets.SlideFromRightIOS}}> | ||
<Stack.Screen | ||
name={Route.FirstScreen} | ||
component={Screen} | ||
options={{title: 'Sample List'}} | ||
/> | ||
<Stack.Screen | ||
name={Route.SecondScreen} | ||
component={Screen} | ||
options={{title: 'Sample List 2'}} | ||
/> | ||
</Stack.Navigator> | ||
</NavigationContainer> | ||
); | ||
}; | ||
|
||
const styles = StyleSheet.create({ | ||
headerWrapper: { | ||
overflow: 'hidden', | ||
}, | ||
footerWrapper: { | ||
paddingVertical: 50 / 3, | ||
paddingHorizontal: 40 / 3, | ||
}, | ||
listItemContainer: { | ||
flexDirection: 'row', | ||
alignItems: 'center', | ||
paddingHorizontal: 50 / 3, | ||
paddingVertical: 40 / 3, | ||
}, | ||
image: { | ||
width: 200 / 3, | ||
aspectRatio: 1, | ||
borderRadius: 25 / 3, | ||
backgroundColor: Colors.primary, | ||
}, | ||
listItemContentWrapper: { | ||
flexDirection: 'column', | ||
paddingHorizontal: 30 / 3, | ||
}, | ||
listItemTitle: { | ||
fontWeight: 'bold', | ||
fontSize: 50 / 3, | ||
}, | ||
separator: { | ||
height: 1 / 3, | ||
flex: 1, | ||
marginHorizontal: 50 / 3, | ||
backgroundColor: '#000', | ||
}, | ||
}); | ||
|
||
export default App; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'd suggest we get rid of this naming policy now that all updtes are executed immediately. There is no need to a method named "markUpdated" if it does not in fact mark anything and just executes the update. What I think would make the most sense is if we remove
markUpdated
andupdateIfNeeded
methods, then instead callperformUpdate
directly. In turn,performUpdate
should domAttached
and other checks (that are currently inupdateIfNeeded
and all the logic fromperformUpdate
should be moved toonUpdate
that can be overridden by native stack implementation hence allow for different transactions to be instantiated etc.