Skip to content

Migrate FloatingMessageCounter to function component #22912

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.

Sign up for GitHub

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

Merged
johnmlee101 merged 2 commits into from
Aug 1, 2023
Merged

Migrate FloatingMessageCounter to function component #22912

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77e3595
Migrate FloatingMessageCounter to function component
AGarciaNY Jul 14, 2023
aa582d8
improved FloatingMessageCounter performance by using useCallback
AGarciaNY Jul 16, 2023
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
108 changes: 50 additions & 58 deletions src/pages/home/report/FloatingMessageCounter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {PureComponent} from 'react';
import React, {useEffect, useMemo, useCallback} from 'react';
import {Animated, View} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../../../styles/styles';
Expand All @@ -7,7 +7,7 @@ import Text from '../../../../components/Text';
import Icon from '../../../../components/Icon';
import * as Expensicons from '../../../../components/Icon/Expensicons';
import themeColors from '../../../../styles/themes/default';
import withLocalize, {withLocalizePropTypes} from '../../../../components/withLocalize';
import useLocalize from '../../../../hooks/useLocalize';
import FloatingMessageCounterContainer from './FloatingMessageCounterContainer';

const propTypes = {
Expand All @@ -16,8 +16,6 @@ const propTypes = {

/** Callback to be called when user clicks the New Messages indicator */
onClick: PropTypes.func,

...withLocalizePropTypes,
};

const defaultProps = {
Expand All @@ -28,73 +26,67 @@ const defaultProps = {
const MARKER_INACTIVE_TRANSLATE_Y = -40;
const MARKER_ACTIVE_TRANSLATE_Y = 10;

class FloatingMessageCounter extends PureComponent {
constructor(props) {
super(props);
this.translateY = new Animated.Value(MARKER_INACTIVE_TRANSLATE_Y);
this.show = this.show.bind(this);
this.hide = this.hide.bind(this);
}

componentDidUpdate() {
if (this.props.isActive) {
this.show();
} else {
this.hide();
}
}
function FloatingMessageCounter(props) {
const {translate} = useLocalize();
const translateY = useMemo(() => new Animated.Value(MARKER_INACTIVE_TRANSLATE_Y), []);

@rushatgabhane rushatgabhane Jul 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: This will calculate the value only once i.e. on mount.


show() {
Animated.spring(this.translateY, {
const show = useCallback(() => {
Animated.spring(translateY, {
toValue: MARKER_ACTIVE_TRANSLATE_Y,
duration: 80,
useNativeDriver: true,
}).start();
}
}, [translateY]);

@rushatgabhane rushatgabhane Jul 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

translateY will never change, right? Why is it in the dependency array?

@AGarciaNY AGarciaNY Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running npm run lint it gives me a warning that useCallback is missing a dependency translateY. It suggests adding it or removing the dependency array. When I remove the dependency array and run lint again it tells me to add an array. I kept translateY as a dependency to keep lint happy 😅.

Would it be best to use something else other than useCallback

@rushatgabhane rushatgabhane Jul 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is fine the.


hide() {
Animated.spring(this.translateY, {
const hide = useCallback(() => {
Animated.spring(translateY, {
toValue: MARKER_INACTIVE_TRANSLATE_Y,
duration: 80,
useNativeDriver: true,
}).start();
}
}, [translateY]);

render() {
return (
<FloatingMessageCounterContainer
accessibilityHint={this.props.translate('accessibilityHints.scrollToNewestMessages')}
containerStyles={[styles.floatingMessageCounterTransformation(this.translateY)]}
>
<View style={styles.floatingMessageCounter}>
<View style={[styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter]}>
<Button
success
small
onPress={this.props.onClick}
>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
<Icon
small
src={Expensicons.DownArrow}
fill={themeColors.textLight}
/>
<Text
selectable={false}
style={[styles.ml2, styles.buttonSmallText, styles.textWhite]}
>
{this.props.translate('newMessages')}
</Text>
</View>
</Button>
</View>
useEffect(() => {
if (props.isActive) {
show();
} else {
hide();
}
}, [props.isActive, show, hide]);

@rushatgabhane rushatgabhane Jul 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good


return (
<FloatingMessageCounterContainer
accessibilityHint={translate('accessibilityHints.scrollToNewestMessages')}
containerStyles={[styles.floatingMessageCounterTransformation(translateY)]}
>
<View style={styles.floatingMessageCounter}>
<View style={[styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter]}>
<Button
success
small
onPress={props.onClick}
>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
<Icon
small
src={Expensicons.DownArrow}
fill={themeColors.textLight}
/>
<Text
selectable={false}
style={[styles.ml2, styles.buttonSmallText, styles.textWhite]}
>
{translate('newMessages')}
</Text>
</View>
</Button>
</View>
</FloatingMessageCounterContainer>
);
}
</View>
</FloatingMessageCounterContainer>
);
}

FloatingMessageCounter.propTypes = propTypes;
FloatingMessageCounter.defaultProps = defaultProps;

export default withLocalize(FloatingMessageCounter);
FloatingMessageCounter.displayName = 'FloatingMessageCounter';
export default React.memo(FloatingMessageCounter);

@rushatgabhane rushatgabhane Jul 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we memoizing the component? Did you notice any performance issues?

@AGarciaNY AGarciaNY Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FloatingMessageCounter used to be PureComopent. Which, I believe that it prevents the components from re-rendering unless their own states or props are changed.

I used React.memo to have the same effect.

@rushatgabhane rushatgabhane Jul 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, i missed that