refactor: example app - #5053
Conversation
MikitasK
left a comment
MikitasK
left a comment
There was a problem hiding this comment.
looks awesome! good job 👍
just a few points to address before merge:
| const resetPreferences = React.useCallback(async () => { | ||
| setShouldUseDynamicTheme(true); | ||
| setIsDarkMode(false); | ||
| setCustomFont(false); | ||
| setRippleEffectEnabled(true); | ||
| setPreferencesVisible(false); |
There was a problem hiding this comment.
what about resetting RTL here as well? currently Reset clears stored preferences but leaves rtl unchanged, so the app can remain in RTL after resetting
I18nManager.forceRTL persists forced direction across restarts
There was a problem hiding this comment.
That one was intentional, RTL in the app requires full app reload. I wanted to keep reset more responsive. That's why no RTL reset. I would keep it that way.
| keyExtractor={({ id }) => id} | ||
| contentContainerStyle={[ | ||
| styles.content, | ||
| { paddingBottom: safeArea.bottom + 16 }, |
There was a problem hiding this comment.
could we also include left & right safe-area insets in horizontal padding?
it's necessary to render list correctly in landscape mode (RN navigation docs)
d553f1e to
803f2c7
Compare
|
|
||
| type SampleId = keyof typeof samples; | ||
|
|
||
| const data = (Object.keys(samples) as SampleId[]).map((id) => ({ |
There was a problem hiding this comment.
@typescript-eslint/no-unsafe-type-assertion is an error across example/ now, so this cast plus OrdersSample.tsx:95 and RootNavigator.tsx:22,68,92 all fail yarn lint after a rebase. Object.entries won't work here - it widens id to string and breaks navigate(item.id). main used eslint-disable where the cast is load-bearing.
| const searchAction = ( | ||
| <Appbar.Action | ||
| icon="folder-search" | ||
| onPress={() => navigation.navigate('ExampleList')} | ||
| /> | ||
| ); | ||
|
|
||
| return ( | ||
| <Appbar.Header elevated> | ||
| {back ? ( | ||
| <Appbar.BackAction onPress={() => navigation.goBack()} /> | ||
| ) : ( | ||
| <Appbar.Action | ||
| icon="menu" | ||
| isLeading | ||
| onPress={() => drawerNavigation.openDrawer()} | ||
| /> | ||
| )} | ||
| <Appbar.Header> | ||
| {back ? backAction : isIOS ? searchAction : null} | ||
| <Appbar.Content title={options.title || route.name} /> | ||
| {!isIOS && !back && searchAction} | ||
| <Appbar.Action icon="cog" onPress={togglePreferences} /> |
There was a problem hiding this comment.
Neither of these actions has an aria-label, and with the drawer gone they're the only way to reach the examples list and preferences. You labelled the Searchbar equivalents one file over in ExampleListHeader.
| const searchAction = ( | ||
| <Appbar.Action | ||
| icon="folder-search" | ||
| onPress={() => navigation.navigate('ExampleList')} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Add isLeading here - on iOS this renders in the leading slot, and AppbarAction uses the prop to pick onSurface over onSurfaceVariant. main passed it on the leading menu action.
There was a problem hiding this comment.
Fixed, my idea was to have to Actions to the right of the Content on Android. Now it's back to Examples navigtiaon to the left on both platforms.
| React.useLayoutEffect(() => { | ||
| navigation.setOptions({ headerShown: false }); | ||
| }, [navigation]); |
There was a problem hiding this comment.
Declare headerShown: false in the screen config instead of hiding it in a layout effect - as written, the stack header mounts and unmounts on the first frame. Add an optional options to SampleConfig and spread it in RootNavigator.
There was a problem hiding this comment.
Fixed, moved it into Config object
803f2c7 to
d09e485
Compare
4 participants
Motivation
PR improves example app. It adds:
Related issue
#4992
Test plan
yarn lint,yarn typecheck,yarn testand a visual check of provided screenshots