Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions dev-client/src/components/Card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,10 @@ export const Card = ({
...boxProps
}: Props) => {
return (
// TODO(performance): Should be investigated in terms of performance as it's what's being
// renedered in the very non-performant FlatList in the HomeScreen's BottomSheet
<Pressable onPress={onPress}>
<Box
variant="card"
marginTop={isPopover ? '15px' : '0px'}
shadow={isPopover ? 9 : undefined}
{...boxProps}>
<Box variant="card" marginTop={'0px'} shadow={undefined} {...boxProps}>
{isPopover && <CardTriangle />}
{children}
{buttons}
Expand Down
33 changes: 27 additions & 6 deletions dev-client/src/components/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
useMemo,
} from 'react';
import {CardCloseButton} from 'terraso-mobile-client/components/CardCloseButton';
import {Pressable, StyleSheet} from 'react-native';
import {View, Pressable, StyleSheet, Modal as RNModal} from 'react-native';
import {useDisclose, Modal as NativeBaseModal} from 'native-base';
import {KeyboardAvoidingView} from 'react-native';

Expand Down Expand Up @@ -69,22 +69,37 @@ export const Modal = forwardRef<ModalHandle, Props>(
{trigger(handle.onOpen)}
</Pressable>
)}
<NativeBaseModal isOpen={isOpen} onClose={onClose}>
{/* // TODO(performance): Fixme
This component adds a significant number of elements to the render tree
even when it's not visible - replacing it with a regular RN Modal for now because of that,
although that partly breaks the functionality and looks different than the original

*/}
{/* <NativeBaseModal isOpen={isOpen} onClose={onClose}> */}
<RNModal
visible={isOpen}
presentationStyle="pageSheet"
animationType="slide"
onDismiss={onClose}
onRequestClose={onClose}>
<KeyboardAvoidingView
style={styles.nativeBaseModalChild}
behavior="padding"
keyboardVerticalOffset={100}>
<NativeBaseModal.Content
{/* <NativeBaseModal.Content
borderRadius="24px"
padding="18px"
{..._content}>
{..._content}> */}
<View style={styles.modalContainer}>
<ModalContext.Provider value={handle}>
{children}
</ModalContext.Provider>
{CloseButton}
</NativeBaseModal.Content>
</View>
{/* </NativeBaseModal.Content> */}
</KeyboardAvoidingView>
</NativeBaseModal>
</RNModal>
{/* </NativeBaseModal> */}
</>
);
},
Expand All @@ -95,4 +110,10 @@ const styles = StyleSheet.create({
width: '100%',
alignItems: 'center',
},
modalContainer: {
// TODO(performance): Adjust styles
backgroundColor: 'white',
padding: 18,
borderRadius: 24,
},
});
15 changes: 8 additions & 7 deletions dev-client/src/components/SiteCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import {useNavigation} from 'terraso-mobile-client/navigation/hooks/useNavigation';
import {Box, Heading, Text, Badge, Row} from 'native-base';
import {useCallback} from 'react';
import React, {useCallback} from 'react';
import {Site} from 'terraso-client-shared/site/siteSlice';
import {useSelector} from 'terraso-mobile-client/store';
import {useTranslation} from 'react-i18next';
Expand All @@ -36,12 +36,7 @@ type Props = {
isPopover?: boolean;
};

export const SiteCard = ({
site,
onShowSiteOnMap,
buttons,
isPopover,
}: Props) => {
const SiteCard = ({site, onShowSiteOnMap, buttons, isPopover}: Props) => {
const {t} = useTranslation();
const navigation = useNavigation();
const project = useSelector(state =>
Expand Down Expand Up @@ -98,4 +93,10 @@ export const SiteCard = ({
);
};

// TODO(performance): Check whether it would be beneficial to memoize this component
// const SiteCardMemoized = React.memo(SiteCard);
// export {SiteCardMemoized};

export {SiteCard};

const styles = StyleSheet.create({mapView: {height: 60, width: 60}});
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {useDispatch} from 'terraso-mobile-client/store';
import {useNavigation} from 'terraso-mobile-client/navigation/hooks/useNavigation';
import {Formik} from 'formik';
import {useTranslation} from 'react-i18next';
import {useMemo} from 'react';
import React, {useMemo} from 'react';
import {PROJECT_DEFAULT_MEASUREMENT_UNITS} from 'terraso-mobile-client/constants';

type Props = {
Expand Down Expand Up @@ -60,25 +60,56 @@ export const CreateProjectForm = ({onInfoPress}: Props) => {
description: '',
privacy: 'PRIVATE',
}}>
{({isSubmitting, handleSubmit}) => (
<KeyboardAvoidingView flex={1}>
<ScrollView bg="background.default">
<Box pt="20%" mx={5}>
<Form onInfoPress={onInfoPress} />
</Box>
</ScrollView>
<Box position="absolute" bottom={8} right={3} p={3}>
<Button
onPress={() => handleSubmit()}
disabled={isSubmitting}
shadow={5}
size={'lg'}
_text={{textTransform: 'uppercase'}}>
{t('general.save_fab')}
</Button>
</Box>
</KeyboardAvoidingView>
{({isSubmitting, handleSubmit, handleChange, handleBlur, values}) => (
<FormContainer
isSubmitting={isSubmitting}
handleSubmit={handleSubmit}
handleChange={handleChange}
handleBlur={handleBlur}
onInfoPress={onInfoPress}
privacy={values.privacy}
/>
)}
</Formik>
);
};

// TODO(performance): Adjust types, think about simplifying the structure here a little bit
// and/or extracting this component to a separate file
const FormContainer = React.memo(
({
isSubmitting,
handleSubmit,
handleChange,
handleBlur,
onInfoPress,
privacy,
}) => {
const {t} = useTranslation();

return (
<KeyboardAvoidingView flex={1}>
<ScrollView bg="background.default">
<Box pt="20%" mx={5}>
<Form
onInfoPress={onInfoPress}
handleChange={handleChange}
handleBlur={handleBlur}
privacy={privacy}
/>
</Box>
</ScrollView>
<Box position="absolute" bottom={8} right={3} p={3}>
<Button
onPress={handleSubmit}
disabled={isSubmitting}
shadow={5}
size={'lg'}
_text={{textTransform: 'uppercase'}}>
{t('general.save_fab')}
</Button>
</Box>
</KeyboardAvoidingView>
);
},
);
27 changes: 20 additions & 7 deletions dev-client/src/screens/CreateProjectScreen/components/Form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
TextArea,
VStack,
} from 'native-base';
import {Formik, useFormikContext} from 'formik';
import {Formik} from 'formik';
import {RadioBlock} from 'terraso-mobile-client/components/RadioBlock';
import {IconButton} from 'terraso-mobile-client/components/Icons';
import {useTranslation} from 'react-i18next';
Expand Down Expand Up @@ -130,6 +130,7 @@ export const EditForm = ({
submitProps,
}: Omit<FormProps, 'privacy'>) => {
const {t} = useTranslation();

return (
<Formik<Omit<ProjectUpdateMutationInput, 'id'>>
validationSchema={editProjectValidationSchema(t)}
Expand Down Expand Up @@ -163,12 +164,17 @@ export const EditForm = ({
);
};

export default function Form({editForm = false, onInfoPress}: Props) {
// TODO(performance): Adjust types
Copy link
Member

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?

Copy link
Collaborator Author

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

export default function Form({
editForm = false,
onInfoPress,
handleChange,
handleBlur,
privacy,
}: Props) {
const {t} = useTranslation();
const {handleChange, handleBlur, values} =
useFormikContext<ProjectFormValues>();

const inputParams = (field: keyof ProjectFormValues) => ({
value: values[field],
onChangeText: handleChange(field),
onBlur: handleBlur(field),
});
Expand All @@ -183,7 +189,13 @@ export default function Form({editForm = false, onInfoPress}: Props) {
<VStack space={3}>
{EditHeader}
<FormLabel>{t('projects.create.name_label')}</FormLabel>
<Input placeholder={t('projects.add.name')} {...inputParams('name')} />
<Input
placeholder={t('projects.add.name')}
defaultValue=""
autoCorrect={false}
scrollEnabled={false}
{...inputParams('name')}
/>
<ErrorMessage fieldName="name" />

<FormLabel>{t('projects.create.description_label')}</FormLabel>
Expand All @@ -192,6 +204,7 @@ export default function Form({editForm = false, onInfoPress}: Props) {
numberOfLines={3}
fontSize={16}
autoCompleteType="off"
autoCorrect={false}
{...inputParams('description')}
/>
<ErrorMessage fieldName="description" />
Expand All @@ -211,7 +224,7 @@ export default function Form({editForm = false, onInfoPress}: Props) {
PRIVATE: {text: t('projects.create.private')},
}}
groupProps={{
value: values.privacy,
value: privacy, // TODO(performance): Investigate whether this input _really_ has to be controlled
variant: 'oneLine',
onChange: handleChange('privacy'),
name: 'data-privacy',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,15 @@ export const CreateSiteView = ({
...parseCoords(coords),
});
if (createdSite !== undefined) {
navigation.replace('HOME', {
site: createdSite,
});
navigation.pop();

// TODO(performance): This is a big no-no as it creates a new HomeScreen
// and adds it to the Stack every time the user creates a new site,
// progressively taking up more and more RAM and choking up the app.
// (Relates to https://github.com/techmatters/terraso-mobile-client/pull/698)
// navigation.replace('HOME', {
// site: createdSite,
// });
}
},
[createSiteCallback, navigation, validationSchema],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ import {SiteFilterModal} from 'terraso-mobile-client/screens/HomeScreen/componen
import {getStartingSnapValue} from 'terraso-mobile-client/screens/HomeScreen/utils/getStartingSnapValue';
import {useListFilter} from 'terraso-mobile-client/components/ListFilter';

// TODO(performance): Same as in ProjectList.tsx
const WINDOW_SIZE = 3;
const MAX_TO_RENDER_PER_BATCH = 3;
// const ITEM_HEIGHT = ??
const SEPARATOR_HEIGHT = 8;

type Props = {
sites: Site[];
showSiteOnMap: (site: Site) => void;
Expand Down Expand Up @@ -77,6 +83,13 @@ export const SiteListBottomSheet = forwardRef<BottomSheetMethods, Props>(

const useDistance = useMemo(() => siteDistances !== null, [siteDistances]);

// TODO(performance): Same as in ProjectList.tsx
// const getItemLayout = useCallback((_: any, index: number) => {
// const itemHeight = ITEM_HEIGHT + SEPARATOR_HEIGHT;
// const offset = itemHeight * index;
// return {length: itemHeight, offset, index};
// }, []);

return (
<BottomSheet
ref={ref}
Expand All @@ -98,9 +111,12 @@ export const SiteListBottomSheet = forwardRef<BottomSheetMethods, Props>(
<BottomSheetFlatList
style={listStyle}
data={filteredSites}
maxToRenderPerBatch={MAX_TO_RENDER_PER_BATCH}
windowSize={WINDOW_SIZE}
// getItemLayout={getItemLayout}
keyExtractor={site => site.id}
renderItem={renderSite}
ItemSeparatorComponent={() => <Box h="8px" />}
ItemSeparatorComponent={() => <Box h={`${SEPARATOR_HEIGHT}px`} />}
ListFooterComponent={<Box h="10px" />}
ListEmptyComponent={<Text>{t('site.search.no_matches')}</Text>}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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>}
/>
Expand Down
Loading