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 mark as complete button shows up on cancelled task report #38991

Merged
merged 6 commits into from
Apr 1, 2024
Merged

Fix mark as complete button shows up on cancelled task report #38991

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
245ae01
Fix get parentReportAction
dukenv0307 Mar 20, 2024
1110e5b
resolve conflict
dukenv0307 Mar 26, 2024
52ee85a
memo parentReportActions
dukenv0307 Mar 26, 2024
05676fb
Merge branch 'main' into fix/37707
dukenv0307 Mar 27, 2024
72e4e19
merge main
dukenv0307 Mar 28, 2024
c94fec8
Merge branch 'main' into fix/37707
dukenv0307 Mar 29, 2024
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
2 changes: 1 addition & 1 deletion src/pages/home/HeaderView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ function HeaderView({report, personalDetails, parentReport, parentReportAction,
)}
</PressableWithoutFeedback>
<View style={[styles.reportOptions, styles.flexRow, styles.alignItemsCenter]}>
{isTaskReport && !isSmallScreenWidth && ReportUtils.isOpenTaskReport(report) && <TaskHeaderActionButton report={report} />}
{isTaskReport && !isSmallScreenWidth && ReportUtils.isOpenTaskReport(report, parentReportAction) && <TaskHeaderActionButton report={report} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

@dukenv0307 this change is enough for me. No need changes in ReportScreen. Am I missing anything?

web-ios.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@situchan #37707 (comment) the change in ReportScreen is added to fix the issue that you mentioned here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, but the current change will cause performance regression on ReportScreen.
I think what I reported is minor and can be out of scope if it's not straightforward fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you see the perf-test is passed since we change the memo to only re-render if the parentReportAction is changed.

{canJoin && !isSmallScreenWidth && joinButton}
{shouldShowThreeDotsButton && (
<ThreeDotsMenu
Expand Down
80 changes: 45 additions & 35 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {InteractionManager, View} from 'react-native';
import type {FlatList, ViewStyle} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import type {WithOnyxInstanceState} from 'react-native-onyx/dist/types';
import type {LayoutChangeEvent} from 'react-native/Libraries/Types/CoreEventTypes';
import Banner from '@components/Banner';
import BlockingView from '@components/BlockingViews/BlockingView';
Expand Down Expand Up @@ -54,7 +53,7 @@ import ReportFooter from './report/ReportFooter';
import {ActionListContext, ReactionListContext} from './ReportScreenContext';
import type {ActionListContextType, ReactionListRef, ScrollPosition} from './ReportScreenContext';

type ReportScreenOnyxProps = {
type ReportScreenOnyxPropsWithoutParentReportAction = {
/** Get modal status */
modal: OnyxEntry<OnyxTypes.Modal>;

Expand Down Expand Up @@ -84,9 +83,6 @@ type ReportScreenOnyxProps = {

/** The report metadata loading states */
reportMetadata: OnyxEntry<OnyxTypes.ReportMetadata>;

/** The report's parentReportAction */
parentReportAction: OnyxEntry<OnyxTypes.ReportAction>;
};

type OnyxHOCProps = {
Expand All @@ -96,7 +92,14 @@ type OnyxHOCProps = {

type ReportScreenNavigationProps = StackScreenProps<CentralPaneNavigatorParamList, typeof SCREENS.REPORT>;

type ReportScreenProps = OnyxHOCProps & CurrentReportIDContextValue & ReportScreenOnyxProps & ReportScreenNavigationProps;
type ReportScreenPropsWithoutParentReportAction = OnyxHOCProps & CurrentReportIDContextValue & ReportScreenOnyxPropsWithoutParentReportAction & ReportScreenNavigationProps;

type ReportScreenParentReportActionOnyxProps = {
/** The report's parentReportActions */
parentReportActions: OnyxEntry<OnyxTypes.ReportActions>;
};

type ReportScreenProps = ReportScreenPropsWithoutParentReportAction & ReportScreenParentReportActionOnyxProps;

/** Get the currently viewed report ID as number */
function getReportID(route: ReportScreenNavigationProps['route']): string {
Expand All @@ -119,6 +122,13 @@ function isEmpty(report: OnyxTypes.Report): boolean {
return !Object.values(report).some((value) => value !== undefined && value !== '');
}

function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, parentReportActionID: string | undefined): OnyxEntry<OnyxTypes.ReportAction> {
if (!parentReportActions || !parentReportActionID) {
return null;
}
return parentReportActions[parentReportActionID ?? '0'];
}

function ReportScreen({
betas = [],
route,
Expand All @@ -129,7 +139,7 @@ function ReportScreen({
isLoadingOlderReportActions: false,
isLoadingNewerReportActions: false,
},
parentReportAction,
parentReportActions,
accountManagerReportID,
markReadyForHydration,
policies = {},
Expand Down Expand Up @@ -235,6 +245,8 @@ function ReportScreen({
],
);

const parentReportAction = useMemo(() => getParentReportAction(parentReportActions, report?.parentReportActionID), [parentReportActions, report.parentReportActionID]);

const prevReport = usePrevious(report);
const prevUserLeavingStatus = usePrevious(userLeavingStatus);
const [isLinkingToMessage, setIsLinkingToMessage] = useState(!!reportActionIDFromRoute);
Expand Down Expand Up @@ -664,7 +676,7 @@ function ReportScreen({
ReportScreen.displayName = 'ReportScreen';

export default withCurrentReportID(
withOnyx<ReportScreenProps, ReportScreenOnyxProps>(
withOnyx<ReportScreenPropsWithoutParentReportAction, ReportScreenOnyxPropsWithoutParentReportAction>(
{
modal: {
key: ONYXKEYS.MODAL,
Expand Down Expand Up @@ -709,36 +721,34 @@ export default withCurrentReportID(
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_USER_IS_LEAVING_ROOM}${getReportID(route)}`,
initialValue: false,
},
parentReportAction: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,
selector: (parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, props: WithOnyxInstanceState<ReportScreenOnyxProps>): OnyxEntry<OnyxTypes.ReportAction> => {
const parentReportActionID = props?.report?.parentReportActionID;
if (!parentReportActionID) {
return null;
}
return parentReportActions?.[parentReportActionID] ?? null;
},
canEvict: false,
},
},
true,
)(
memo(
ReportScreen,
(prevProps, nextProps) =>
prevProps.isSidebarLoaded === nextProps.isSidebarLoaded &&
lodashIsEqual(prevProps.sortedAllReportActions, nextProps.sortedAllReportActions) &&
lodashIsEqual(prevProps.reportMetadata, nextProps.reportMetadata) &&
prevProps.isComposerFullSize === nextProps.isComposerFullSize &&
lodashIsEqual(prevProps.betas, nextProps.betas) &&
lodashIsEqual(prevProps.policies, nextProps.policies) &&
prevProps.accountManagerReportID === nextProps.accountManagerReportID &&
prevProps.userLeavingStatus === nextProps.userLeavingStatus &&
prevProps.currentReportID === nextProps.currentReportID &&
lodashIsEqual(prevProps.modal, nextProps.modal) &&
lodashIsEqual(prevProps.parentReportAction, nextProps.parentReportAction) &&
lodashIsEqual(prevProps.route, nextProps.route) &&
lodashIsEqual(prevProps.report, nextProps.report),
withOnyx<ReportScreenProps, ReportScreenParentReportActionOnyxProps>({
parentReportActions: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,
canEvict: false,
},
})(
memo(ReportScreen, (prevProps, nextProps) => {
const prevParentReportAction = getParentReportAction(prevProps.parentReportActions, prevProps.report?.parentReportActionID);
const nextParentReportAction = getParentReportAction(nextProps.parentReportActions, nextProps.report?.parentReportActionID);
return (
prevProps.isSidebarLoaded === nextProps.isSidebarLoaded &&
lodashIsEqual(prevProps.sortedAllReportActions, nextProps.sortedAllReportActions) &&
lodashIsEqual(prevProps.reportMetadata, nextProps.reportMetadata) &&
prevProps.isComposerFullSize === nextProps.isComposerFullSize &&
lodashIsEqual(prevProps.betas, nextProps.betas) &&
lodashIsEqual(prevProps.policies, nextProps.policies) &&
prevProps.accountManagerReportID === nextProps.accountManagerReportID &&
prevProps.userLeavingStatus === nextProps.userLeavingStatus &&
prevProps.currentReportID === nextProps.currentReportID &&
lodashIsEqual(prevProps.modal, nextProps.modal) &&
lodashIsEqual(prevParentReportAction, nextParentReportAction) &&
lodashIsEqual(prevProps.route, nextProps.route) &&
lodashIsEqual(prevProps.report, nextProps.report)
);
}),
),
),
);
Loading