-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: performance improvements #787
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,14 @@ export const CreateSiteForm = ({ | |
<VStack p="16px" pt="30px" space="18px"> | ||
<FormField name="name"> | ||
<FormLabel>{t('site.create.name_label')}</FormLabel> | ||
|
||
{/* TODO(performance): | ||
1. All text inputs should be adjusted into uncontrolled components | ||
- https://react.dev/learn/sharing-state-between-components#controlled-and-uncontrolled-components | ||
- https://github.com/facebook/react-native/issues/20119 | ||
2. The code should be rewritten/reorganized/memoized so that a change to a text input's value doesn't | ||
trigger a re-render of the whole Form/Component (chokes the JS thread and results in terrible frame drops) | ||
*/} | ||
Comment on lines
+92
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you expand on how this would work? the form components aren't controlled by props when they're in a form, and are memoized. or are you saying that the entire architecture of Formik is causing the forms to re-render on each keystroke and we need to move away from the library? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll try to elaborate on this further next week |
||
<FormInput placeholder={t('site.create.name_placeholder')} /> | ||
</FormField> | ||
<FormField name="coords"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,20 +16,49 @@ | |
*/ | ||
|
||
import {Box, FlatList, Text} from 'native-base'; | ||
import {useCallback} from 'react'; | ||
import {useTranslation} from 'react-i18next'; | ||
|
||
import {Project} from 'terraso-client-shared/project/projectSlice'; | ||
import {useListFilter} from 'terraso-mobile-client/components/ListFilter'; | ||
import {ProjectPreviewCard} from 'terraso-mobile-client/screens/ProjectListScreen/components/ProjectPreviewCard'; | ||
|
||
// TODO(performance): | ||
// Some relevant reading: https://reactnative.dev/docs/optimizing-flatlist-configuration#windowsize | ||
const WINDOW_SIZE = 6; // Default is 21, bringing it down as the items in this FlatList are very costly to render | ||
const MAX_TO_RENDER_PER_BATCH = 5; // Similar as above, default is 10 | ||
// const ITEM_HEIGHT = ?? | ||
const SEPARATOR_HEIGHT = 8; | ||
|
||
type RenderItemProps = { | ||
item: Project; | ||
}; | ||
|
||
export const ProjectList = () => { | ||
const {t} = useTranslation(); | ||
const {filteredItems} = useListFilter<Project>(); | ||
|
||
const renderItem = useCallback( | ||
({item}: RenderItemProps) => <ProjectPreviewCard project={item} />, | ||
[], | ||
); | ||
|
||
// TODO(performance): Consider adjusting ProjectPreviewCard with the design team | ||
// to always have a fixed height so you can leverage getItemLayout (example below): | ||
// const getItemLayout = useCallback((_: any, index: number) => { | ||
// const length = ITEM_HEIGHT + SEPARATOR_HEIGHT; | ||
// const offset = length * index; | ||
// return {length, offset, index}; | ||
// }, []); | ||
Comment on lines
+46
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is kind of a general UI question, but if you're specifying fixed heights in pixels in advance, how do you handle something like a user who has set a large font size preference for accessibility reasons? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to look into that, but off the top of my head, I think it doesn't have to be fixed-fixed but rather deterministic and repeated for all items, great question though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see, so it'd work but you'd need to find a way to compute that dynamically. i've run into this problem with other UI frameworks before, never understood why there can't be an option to say "i promise they'll all be the same dimension" instead of "i promise they'll all be this dimension" so the framework can just measure one item's layout and use that value for the rest of the list which seems much less error-prone |
||
|
||
return ( | ||
<FlatList | ||
data={filteredItems} | ||
renderItem={({item}) => <ProjectPreviewCard project={item} />} | ||
ItemSeparatorComponent={() => <Box h="8px" />} | ||
renderItem={renderItem} | ||
windowSize={WINDOW_SIZE} | ||
maxToRenderPerBatch={MAX_TO_RENDER_PER_BATCH} | ||
// getItemLayout={getItemLayout} | ||
ItemSeparatorComponent={() => <Box h={`${SEPARATOR_HEIGHT}px`} />} | ||
keyExtractor={project => project.id} | ||
ListEmptyComponent={<Text>{t('projects.search.no_matches')}</Text>} | ||
/> | ||
|
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.
What sort of adjustments are needed?
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.
Nothing major, just regular props type adjustments I omitted here to save a bit of time