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.

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

Jump to bottom

Refactor HeaderView component #31671

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Refactor HeaderView component #31671

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
93 changes: 48 additions & 45 deletions src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,12 @@ import TaskHeaderActionButton from '@components/TaskHeaderActionButton';
import Text from '@components/Text';
import ThreeDotsMenu from '@components/ThreeDotsMenu';
import Tooltip from '@components/Tooltip';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
import withWindowDimensions, {windowDimensionsPropTypes} from '@components/withWindowDimensions';
import compose from '@libs/compose';
import useLocalize from '@hooks/useLocalize';
import useWindowDimensions from '@hooks/useWindowDimensions';
import {getGroupChatName} from '@libs/GroupChatUtils';
import * as HeaderUtils from '@libs/HeaderUtils';
import reportWithoutHasDraftSelector from '@libs/OnyxSelectors/reportWithoutHasDraftSelector';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import reportPropTypes from '@pages/reportPropTypes';
Expand Down Expand Up @@ -60,8 +58,14 @@ const propTypes = {
accountID: PropTypes.number,
}),

...windowDimensionsPropTypes,
...withLocalizePropTypes,
/** The current policy of the report */
policy: PropTypes.shape({
/** The policy name */
name: PropTypes.string,

/** The URL for the policy avatar */
avatar: PropTypes.string,
}),
};

const defaultProps = {
Expand All @@ -72,9 +76,12 @@ const defaultProps = {
session: {
accountID: 0,
},
policy: {},
};

function HeaderView(props) {
const {isSmallScreenWidth, windowWidth} = useWindowDimensions();
const {translate} = useLocalize();
const theme = useTheme();
const styles = useThemeStyles();
const participants = lodashGet(props.report, 'participantAccountIDs', []);
Expand All @@ -97,10 +104,9 @@ function HeaderView(props) {
const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(props.report.reportID);
const isEmptyChat = !props.report.lastMessageText && !props.report.lastMessageTranslationKey && !lastVisibleMessage.lastMessageText && !lastVisibleMessage.lastMessageTranslationKey;
const isUserCreatedPolicyRoom = ReportUtils.isUserCreatedPolicyRoom(props.report);
const policy = useMemo(() => props.policies[`${ONYXKEYS.COLLECTION.POLICY}${props.report.policyID}`], [props.policies, props.report.policyID]);
const canLeaveRoom = ReportUtils.canLeaveRoom(props.report, !_.isEmpty(policy));
const isPolicyMember = useMemo(() => !_.isEmpty(props.policy), [props.policy]);
const canLeaveRoom = ReportUtils.canLeaveRoom(props.report, isPolicyMember);
const isArchivedRoom = ReportUtils.isArchivedRoom(props.report);
const isPolicyMember = useMemo(() => PolicyUtils.isPolicyMember(props.report.policyID, props.policies), [props.report.policyID, props.policies]);

// We hide the button when we are chatting with an automated Expensify account since it's not possible to contact
// these users via alternative means. It is possible to request a call with Concierge so we leave the option for them.
Expand All @@ -112,7 +118,7 @@ function HeaderView(props) {
if (ReportUtils.isCompletedTaskReport(props.report) && canModifyTask) {
threeDotMenuItems.push({
icon: Expensicons.Checkmark,
text: props.translate('task.markAsIncomplete'),
text: translate('task.markAsIncomplete'),
onSelected: Session.checkIfActionIsAllowed(() => Task.reopenTask(props.report)),
});
}
Expand All @@ -121,7 +127,7 @@ function HeaderView(props) {
if (props.report.stateNum !== CONST.REPORT.STATE_NUM.SUBMITTED && props.report.statusNum !== CONST.REPORT.STATUS.CLOSED && canModifyTask) {
threeDotMenuItems.push({
icon: Expensicons.Trashcan,
text: props.translate('common.cancel'),
text: translate('common.cancel'),
onSelected: Session.checkIfActionIsAllowed(() => Task.cancelTask(props.report.reportID, props.report.reportName, props.report.stateNum, props.report.statusNum)),
});
}
Expand All @@ -131,7 +137,7 @@ function HeaderView(props) {
if (props.report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
threeDotMenuItems.push({
icon: Expensicons.ChatBubbles,
text: props.translate('common.join'),
text: translate('common.join'),
onSelected: Session.checkIfActionIsAllowed(() =>
Report.updateNotificationPreference(props.report.reportID, props.report.notificationPreference, CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS, false),
),
Expand All @@ -140,7 +146,7 @@ function HeaderView(props) {
const isWorkspaceMemberLeavingWorkspaceRoom = lodashGet(props.report, 'visibility', '') === CONST.REPORT.VISIBILITY.RESTRICTED && isPolicyMember;
threeDotMenuItems.push({
icon: Expensicons.ChatBubbles,
text: props.translate('common.leave'),
text: translate('common.leave'),
onSelected: Session.checkIfActionIsAllowed(() => Report.leaveRoom(props.report.reportID, isWorkspaceMemberLeavingWorkspaceRoom)),
});
}
Expand All @@ -151,22 +157,22 @@ function HeaderView(props) {
if (isConcierge && props.guideCalendarLink) {
threeDotMenuItems.push({
icon: Expensicons.Phone,
text: props.translate('videoChatButtonAndMenu.tooltip'),
text: translate('videoChatButtonAndMenu.tooltip'),
onSelected: Session.checkIfActionIsAllowed(() => {
Link.openExternalLink(props.guideCalendarLink);
}),
});
} else if (!isAutomatedExpensifyAccount && !isTaskReport && !isArchivedRoom) {
threeDotMenuItems.push({
icon: ZoomIcon,
text: props.translate('videoChatButtonAndMenu.zoom'),
text: translate('videoChatButtonAndMenu.zoom'),
onSelected: Session.checkIfActionIsAllowed(() => {
Link.openExternalLink(CONST.NEW_ZOOM_MEETING_URL);
}),
});
threeDotMenuItems.push({
icon: GoogleMeetIcon,
text: props.translate('videoChatButtonAndMenu.googleMeet'),
text: translate('videoChatButtonAndMenu.googleMeet'),
onSelected: Session.checkIfActionIsAllowed(() => {
Link.openExternalLink(CONST.NEW_GOOGLE_MEET_MEETING_URL);
}),
Expand All @@ -179,7 +185,7 @@ function HeaderView(props) {
const defaultSubscriptSize = ReportUtils.isExpenseRequest(props.report) ? CONST.AVATAR_SIZE.SMALL_NORMAL : CONST.AVATAR_SIZE.DEFAULT;
const icons = ReportUtils.getIcons(reportHeaderData, props.personalDetails);
const brickRoadIndicator = ReportUtils.hasReportNameError(props.report) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';
const shouldShowBorderBottom = !isTaskReport || !props.isSmallScreenWidth;
const shouldShowBorderBottom = !isTaskReport || !isSmallScreenWidth;
const shouldDisableDetailPage = ReportUtils.shouldDisableDetailPage(props.report);

const isLoading = !props.report || !title;
Expand All @@ -189,21 +195,21 @@ function HeaderView(props) {
style={[styles.appContentHeader, shouldShowBorderBottom && styles.borderBottom]}
dataSet={{dragArea: true}}
>
<View style={[styles.appContentHeaderTitle, !props.isSmallScreenWidth && !isLoading && styles.pl5]}>
<View style={[styles.appContentHeaderTitle, !isSmallScreenWidth && !isLoading && styles.pl5]}>
{isLoading ? (
<ReportHeaderSkeletonView />
) : (
<>
{props.isSmallScreenWidth && (
{isSmallScreenWidth && (
<PressableWithoutFeedback
onPress={props.onNavigationMenuButtonClicked}
style={[styles.LHNToggle]}
accessibilityHint={props.translate('accessibilityHints.navigateToChatsList')}
accessibilityLabel={props.translate('common.back')}
accessibilityHint={translate('accessibilityHints.navigateToChatsList')}
accessibilityLabel={translate('common.back')}
role={CONST.ACCESSIBILITY_ROLE.BUTTON}
>
<Tooltip
text={props.translate('common.back')}
text={translate('common.back')}
shiftVertical={4}
>
<View>
Expand Down Expand Up @@ -267,10 +273,10 @@ function HeaderView(props) {
)}
</PressableWithoutFeedback>
<View style={[styles.reportOptions, styles.flexRow, styles.alignItemsCenter]}>
{isTaskReport && !props.isSmallScreenWidth && ReportUtils.isOpenTaskReport(props.report) && <TaskHeaderActionButton report={props.report} />}
{isTaskReport && !isSmallScreenWidth && ReportUtils.isOpenTaskReport(props.report) && <TaskHeaderActionButton report={props.report} />}
{shouldShowThreeDotsButton && (
<ThreeDotsMenu
anchorPosition={styles.threeDotsPopoverOffset(props.windowWidth)}
anchorPosition={styles.threeDotsPopoverOffset(windowWidth)}
menuItems={threeDotMenuItems}
shouldSetModalVisibility={false}
/>
Expand All @@ -288,25 +294,22 @@ HeaderView.displayName = 'HeaderView';
HeaderView.defaultProps = defaultProps;

export default memo(
compose(
withWindowDimensions,
withLocalize,
withOnyx({
guideCalendarLink: {
key: ONYXKEYS.ACCOUNT,
selector: (account) => (account && account.guideCalendarLink) || null,
initialValue: null,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID || report.reportID}`,
selector: reportWithoutHasDraftSelector,
},
session: {
key: ONYXKEYS.SESSION,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
},
}),
)(HeaderView),
withOnyx({
guideCalendarLink: {
key: ONYXKEYS.ACCOUNT,
selector: (account) => (account && account.guideCalendarLink) || null,
initialValue: null,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID || report.reportID}`,
selector: reportWithoutHasDraftSelector,
},
session: {
key: ONYXKEYS.SESSION,
},
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
selector: (policy) => _.pick(policy, ['name', 'avatar', 'pendingAction']),
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: doesn't this break the memoization (this is wrapped by memo()) if this selector is always returning a new instance of the sub-object picked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only pick some fields to prevent unnecessary re-render. Does this break something or cause regression?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this break something or cause regression?

No that I know of, I just saw the code and got curious. I think it would be good to know if it makes the memo completely useless. If it does, it may have been better to just return the complete policy object 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that the implementation of memo is not doing a deep comparison, it is just doing ===, but maybe I'm wrong about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that fine because we are also using only memo in other places without callback comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that fine because we are also using only memo in other places without callback comparison.

It could also be wrong there... I guess I'll have to test it :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, let me know if it has any problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to test and I confirmed that I'm wrong, the memo is working fine.

In case you are interested, I tested by adding this code in ReportScreen to force it to re-render every second

    const [forceRerender, setForceRerender] = useState(0);
    useEffect(() => {
        setTimeout(()=>{
            setForceRerender(forceRerender + 1);
        }, 1000);
    }, [forceRerender])
    console.log('ReportScreen rerender', forceRerender);

and also added a console log in HeaderView. I can see the ReportScreen rerender every second, but HeaderView rerender doesn't appear. If I remove the memo, the HeaderView rerender starts appearing every time ReportScreen rerenders.

},
})(HeaderView),
);
Loading