Skip to content

Refactor ParticipantLocalTime to function component #20300

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
roryabraham merged 2 commits into from
Jun 6, 2023
Merged

Refactor ParticipantLocalTime to function component #20300

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f1a771
Refactor to function component
aldo-expensify Jun 6, 2023
bf72be5
Use clearInterval
aldo-expensify Jun 6, 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
86 changes: 39 additions & 47 deletions src/pages/home/report/ParticipantLocalTime.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, {useState, useEffect} from 'react';
import {View} from 'react-native';
import lodashGet from 'lodash/get';
import styles from '../../../styles/styles';
Expand All @@ -16,60 +16,52 @@ const propTypes = {
...withLocalizePropTypes,
};

class ParticipantLocalTime extends PureComponent {
constructor(props) {
super(props);
this.getParticipantLocalTime = this.getParticipantLocalTime.bind(this);
this.state = {
localTime: this.getParticipantLocalTime(),
};
function getParticipantLocalTime(participant, preferredLocale) {
const reportRecipientTimezone = lodashGet(participant, 'timezone', CONST.DEFAULT_TIME_ZONE);
const reportTimezone = DateUtils.getLocalMomentFromDatetime(preferredLocale, null, reportRecipientTimezone.selected);
const currentTimezone = DateUtils.getLocalMomentFromDatetime(preferredLocale);
const reportRecipientDay = reportTimezone.format('dddd');
const currentUserDay = currentTimezone.format('dddd');

if (reportRecipientDay !== currentUserDay) {
return `${reportTimezone.format('LT')} ${reportRecipientDay}`;
}
return `${reportTimezone.format('LT')}`;
}

function ParticipantLocalTime(props) {
const {participant, preferredLocale, translate} = props;

@fedirjh fedirjh Jun 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should avoid props destruction as per Style guidelines

@aldo-expensify aldo-expensify Jun 6, 2023 •
edited
Loading

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.

This is to avoid a warning on the useEffect watched values:

Screenshot 2023-06-05 at 12 54 07 PM

I think that guideline shouldn't apply well to function components with useEffect

@fedirjh fedirjh Jun 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems we have a similar cases that got resolved with a partial destruction , you can check this thread https://expensify.slack.com/archives/C01GTK53T8Q/p1684969645381109

@aldo-expensify aldo-expensify Jun 6, 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.

Are you suggesting I shouldn't destructure translate because that is not watched and we can keep accessing it as props.translate?

If that is the case, I don't have strong feeling toward that, but I think the best would be to have access to the translate function through a hook like it was suggested here: https://expensify.slack.com/archives/C01GTK53T8Q/p1685068222924839?thread_ts=1684969645.381109&cid=C01GTK53T8Q

@fedirjh fedirjh Jun 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you suggesting I shouldn't destructure translate because that is not watched and we can keep accessing it as props.translate?

Sorry missed this comment. Well, props.translate should be watched as well, as the language may change during re-renders.

I suggested to proceed with the current destruction of props as you did.


componentDidMount() {
this.timer = Timers.register(
const [localTime, setLocalTime] = useState(() => getParticipantLocalTime(participant, preferredLocale));
useEffect(() => {
const timer = Timers.register(
setInterval(() => {
this.setState({
localTime: this.getParticipantLocalTime(),
});
setLocalTime(getParticipantLocalTime(participant, preferredLocale));
}, 1000),
);
}

componentWillUnmount() {
clearInterval(this.timer);
}

getParticipantLocalTime() {
const reportRecipientTimezone = lodashGet(this.props.participant, 'timezone', CONST.DEFAULT_TIME_ZONE);
const reportTimezone = DateUtils.getLocalMomentFromDatetime(this.props.preferredLocale, null, reportRecipientTimezone.selected);
const currentTimezone = DateUtils.getLocalMomentFromDatetime(this.props.preferredLocale);
const reportRecipientDay = reportTimezone.format('dddd');
const currentUserDay = currentTimezone.format('dddd');

if (reportRecipientDay !== currentUserDay) {
return `${reportTimezone.format('LT')} ${reportRecipientDay}`;
}
return `${reportTimezone.format('LT')}`;
}
return () => {
clearInterval(timer);
};
}, [participant, preferredLocale]);

render() {
const reportRecipientDisplayName = lodashGet(this.props, 'participant.firstName') || lodashGet(this.props, 'participant.displayName');
const reportRecipientDisplayName = lodashGet(props, 'participant.firstName') || lodashGet(props, 'participant.displayName');

@fedirjh fedirjh Jun 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aldo-expensify Shouldn’t we wrap it with useMemo ? the state update will re-render the component every second.

@aldo-expensify aldo-expensify Jun 6, 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.

Not worth it in my opinion.

useMemo doesn't prevent re-rendering.

Caching lodashGet(props, 'participant.firstName') || lodashGet(props, 'participant.displayName'); is probably more costly than recalculating it.

@fedirjh fedirjh Jun 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is probably more costly than recalculating it.

hmmm , make sense 👍🏼

@aldo-expensify aldo-expensify Jun 6, 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.

the state update will re-render the component every second.

I just understood what you meant here. The interval is updating the state every second. I'll check if setting the state to the same value triggers a re-render or not. If it does, we should only set the state if it is really different.

@aldo-expensify aldo-expensify Jun 6, 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.

Setting the state to the same value doesn't trigger a re-render:

image

@fedirjh fedirjh Jun 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you , that’s perfect.


return (
<View style={[styles.chatItemComposeSecondaryRow]}>
<Text
style={[styles.chatItemComposeSecondaryRowSubText, styles.chatItemComposeSecondaryRowOffset, styles.pre]}
numberOfLines={1}
>
{this.props.translate('reportActionCompose.localTime', {
user: reportRecipientDisplayName,
time: this.state.localTime,
})}
</Text>
</View>
);
}
return (
<View style={[styles.chatItemComposeSecondaryRow]}>
<Text
style={[styles.chatItemComposeSecondaryRowSubText, styles.chatItemComposeSecondaryRowOffset, styles.pre]}
numberOfLines={1}
>
{translate('reportActionCompose.localTime', {
user: reportRecipientDisplayName,
time: localTime,
})}
</Text>
</View>
);
}

ParticipantLocalTime.propTypes = propTypes;
ParticipantLocalTime.displayName = 'ParticipantLocalTime';

export default withLocalize(ParticipantLocalTime);