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

fix: Remove withCurrentReportID HOC #54702

Merged
merged 14 commits into from
Jan 8, 2025
Merged

fix: Remove withCurrentReportID HOC #54702

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0bd8984
remove with withCurrentReportID from ReportScreen
etCoderDysto Dec 31, 2024
cf74dcc
remove withCurrentReportID HOC from withCurrentReportID file
etCoderDysto Dec 31, 2024
34508e9
Merge remote-tracking branch 'upstream/main' into remove-withCurrentR…
etCoderDysto Jan 1, 2025
7edd16a
fix lint
etCoderDysto Jan 1, 2025
575a66d
fix typecheck
etCoderDysto Jan 1, 2025
a024e36
move CurrentReportIDContextProvider and all other code blocks to useC…
etCoderDysto Jan 7, 2025
d248160
use CurrentReportIDContextProvider exported from useCurrentReportID i…
etCoderDysto Jan 7, 2025
dc4c422
move named exports at the bottom of the file
etCoderDysto Jan 7, 2025
6408eb7
use CurrentReportIDContextProvider from useCurrentReportID hook
etCoderDysto Jan 7, 2025
ba9e6de
remove CurrentReportIDContext from useCurrentReportID named exports s…
etCoderDysto Jan 7, 2025
5d6082f
remove withCurrentReportIDDefaultProps from useCurrentReportID since …
etCoderDysto Jan 7, 2025
952a9c3
CurrentReportIDContextValue from useCurrentReportId
etCoderDysto Jan 7, 2025
cca8360
delete withCurrentReportID hoc
etCoderDysto Jan 7, 2025
3a3b5d5
Merge branch 'main' into remove-withCurrentReportID-usage
etCoderDysto Jan 7, 2025
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
34 changes: 4 additions & 30 deletions src/components/withCurrentReportID.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import type {NavigationState} from '@react-navigation/native';
import type {ComponentType, ForwardedRef, RefAttributes} from 'react';
import React, {createContext, forwardRef, useCallback, useMemo, useState} from 'react';
import getComponentDisplayName from '@libs/getComponentDisplayName';
import React, {createContext, useCallback, useMemo, useState} from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the idea to just get rid of this whole file? I only see one place using updateCurrentReportID, which could also be removed if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luacmartins I don't think we can remove updateCurrentReportID. We are using updateCurrentReportID to update the currentReportID when there is a navigation state change.
That helps sync the LHN item focused on LHN, and the currently opened report. If we don't do that, when select an item from LHN, the corresponding report will be opened but the LHN item won't be focused. The issue video is can be found at the bottom.

  1. We call updateCurrentReportID with the new navigation state


App/src/libs/Navigation/NavigationRoot.tsx

Lines 172 to 173 in 04cd7b0

setTimeout(() => {
currentReportIDValue?.updateCurrentReportID(state);

  1. Then we get the new navigation’s reportID here and updated currentReportID with the new navigation state reportID

App/src/components/withCurrentReportID.tsx

Lines 28 to 30 in 575a66d

const updateCurrentReportID = useCallback(
(state: NavigationState) => {
const reportID = Navigation.getTopmostReportId(state);

  1. Then LHN item will be updated with the new currentReportID, and the right item will be focused. 


    App/src/components/LHNOptionsList/OptionRowLHNData.tsx

    Line 36 in 04cd7b0

    const isReportFocused = isFocused && currentReportIDValue?.currentReportID === reportID;

But, I believe we can move CurrentReportIDContextProviderfrom withCurrentReportID.tsx to useCurrentReportID.tsx and delete withCurrentReportID.tsx file. Should I do that?

POC

Screen.Recording.2025-01-03.at.1.49.03.in.the.afternoon.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think we'd want to move from the HOC to the hook fully. Let's migrate that code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I will do that.

import Navigation from '@libs/Navigation/Navigation';

type CurrentReportIDContextValue = {
updateCurrentReportID: (state: NavigationState) => void;
currentReportID: string;
currentReportID: string | undefined;
};

type CurrentReportIDContextProviderProps = {
Expand All @@ -21,15 +19,15 @@ const withCurrentReportIDDefaultProps = {
};

function CurrentReportIDContextProvider(props: CurrentReportIDContextProviderProps) {
const [currentReportID, setCurrentReportID] = useState('');
const [currentReportID, setCurrentReportID] = useState<string | undefined>('');

/**
* This function is used to update the currentReportID
* @param state root navigation state
*/
const updateCurrentReportID = useCallback(
(state: NavigationState) => {
const reportID = Navigation.getTopmostReportId(state) ?? '-1';
const reportID = Navigation.getTopmostReportId(state);

/*
* Make sure we don't make the reportID undefined when switching between the chat list and settings tab.
Expand Down Expand Up @@ -61,29 +59,5 @@ function CurrentReportIDContextProvider(props: CurrentReportIDContextProviderPro

CurrentReportIDContextProvider.displayName = 'CurrentReportIDContextProvider';

export default function withCurrentReportID<TProps extends CurrentReportIDContextValue, TRef>(
WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>,
): (props: Omit<TProps, keyof CurrentReportIDContextValue> & React.RefAttributes<TRef>) => React.ReactElement | null {
function WithCurrentReportID(props: Omit<TProps, keyof CurrentReportIDContextValue>, ref: ForwardedRef<TRef>) {
return (
<CurrentReportIDContext.Consumer>
{(currentReportIDUtils) => (
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
{...currentReportIDUtils}
// eslint-disable-next-line react/jsx-props-no-spreading
{...(props as TProps)}
ref={ref}
/>
)}
</CurrentReportIDContext.Consumer>
);
}

WithCurrentReportID.displayName = `withCurrentReportID(${getComponentDisplayName(WrappedComponent)})`;

return forwardRef(WithCurrentReportID);
}

export {withCurrentReportIDDefaultProps, CurrentReportIDContextProvider, CurrentReportIDContext};
export type {CurrentReportIDContextValue};
2 changes: 1 addition & 1 deletion src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ function BottomTabBar({selectedTab}: BottomTabBarProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const {activeWorkspaceID} = useActiveWorkspace();
const {currentReportID} = useCurrentReportID() ?? {currentReportID: null};
const {currentReportID = null} = useCurrentReportID() ?? {};
const [user] = useOnyx(ONYXKEYS.USER);
const [betas] = useOnyx(ONYXKEYS.BETAS);
const [priorityMode] = useOnyx(ONYXKEYS.NVP_PRIORITY_MODE);
Expand Down
9 changes: 5 additions & 4 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import ReportActionsSkeletonView from '@components/ReportActionsSkeletonView';
import ScreenWrapper from '@components/ScreenWrapper';
import TaskHeaderActionButton from '@components/TaskHeaderActionButton';
import type {CurrentReportIDContextValue} from '@components/withCurrentReportID';
import withCurrentReportID from '@components/withCurrentReportID';
import useActiveWorkspace from '@hooks/useActiveWorkspace';
import useAppFocusEvent from '@hooks/useAppFocusEvent';
import useCurrentReportID from '@hooks/useCurrentReportID';
import useDeepCompareRef from '@hooks/useDeepCompareRef';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
Expand Down Expand Up @@ -97,7 +97,7 @@ function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportAc
return parentReportActions[parentReportActionID ?? '0'];
}

function ReportScreen({route, currentReportID = '', navigation}: ReportScreenProps) {
function ReportScreen({route, navigation}: ReportScreenProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const reportIDFromRoute = getReportID(route);
Expand All @@ -112,6 +112,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
const {isOffline} = useNetwork();
const {shouldUseNarrowLayout, isInNarrowPaneModal} = useResponsiveLayout();
const {activeWorkspaceID} = useActiveWorkspace();
const currentReportIDValue = useCurrentReportID();

const [modal] = useOnyx(ONYXKEYS.MODAL);
const [isComposerFullSize] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${reportIDFromRoute}`, {initialValue: false});
Expand Down Expand Up @@ -277,7 +278,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
const lastReportAction = [...combinedReportActions, parentReportAction].find((action) => ReportUtils.canEditReportAction(action) && !ReportActionsUtils.isMoneyRequestAction(action));
const isSingleTransactionView = ReportUtils.isMoneyRequest(report) || ReportUtils.isTrackExpenseReport(report);
const policy = policies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID ?? '-1'}`];
const isTopMostReportId = currentReportID === reportIDFromRoute;
const isTopMostReportId = currentReportIDValue?.currentReportID === reportIDFromRoute;
const didSubscribeToReportLeavingEvents = useRef(false);
const [showSoftInputOnFocus, setShowSoftInputOnFocus] = useState(false);

Expand Down Expand Up @@ -870,4 +871,4 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
}

ReportScreen.displayName = 'ReportScreen';
export default withCurrentReportID(memo(ReportScreen, (prevProps, nextProps) => prevProps.currentReportID === nextProps.currentReportID && lodashIsEqual(prevProps.route, nextProps.route)));
export default memo(ReportScreen, (prevProps, nextProps) => lodashIsEqual(prevProps.route, nextProps.route));
Loading