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 editing the message #24154

Closed
wants to merge 16 commits into from
Closed

Fix editing the message #24154

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
78b9f1d
create context to freeze scroll when editing the message and turn off…
Piotrfj Aug 4, 2023
85b5450
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Aug 14, 2023
9ff6e31
refactor of ScrollFrozen context
Piotrfj Aug 21, 2023
45e4736
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Aug 22, 2023
7cb0e91
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Aug 28, 2023
747b378
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Aug 29, 2023
739a7d0
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Aug 30, 2023
23b8e40
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Sep 15, 2023
7936593
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Sep 18, 2023
3a93dd3
bring animation of the scroll back
Piotrfj Sep 21, 2023
ce13894
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Sep 21, 2023
592ecf3
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Sep 22, 2023
292a4a6
fix indents
Piotrfj Sep 22, 2023
bd95a39
fix import
Piotrfj Sep 25, 2023
66ded3b
Merge branch 'main' of https://github.com/Expensify/App into fix/1880…
Piotrfj Sep 25, 2023
ae3fab6
prettier fix
Piotrfj Sep 28, 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
10 changes: 10 additions & 0 deletions src/hooks/useFrozenScroll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import {useContext} from 'react';
import {ReportActionListFrozenScrollContext} from "../pages/home/report/ReportActionListFrozenScrollContext";

/**
* Hook for getting current state of scroll freeze and a function to set whether the scroll should be frozen
* @returns {Object}
*/
export default function useFrozenScroll() {
return useContext(ReportActionListFrozenScrollContext);
Copy link
Contributor

@ArekChr ArekChr Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could improve error handling here.

Suggested change
return useContext(ReportActionListFrozenScrollContext);
const context = useContext(ReportActionListFrozenScrollContext);
if (context === undefined)
throw new Error('useFrozenScroll must be used within ReportActionListFrozenScrollContextProvider');
}
return context;

}
203 changes: 105 additions & 98 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import ReportScreenContext from './ReportScreenContext';
import TaskHeaderActionButton from '../../components/TaskHeaderActionButton';
import DragAndDropProvider from '../../components/DragAndDrop/Provider';
import usePrevious from '../../hooks/usePrevious';
import {ReportActionListFrozenScrollContextProvider} from "./report/ReportActionListFrozenScrollContext";

const propTypes = {
/** Navigation route context info provided by react navigation */
Expand Down Expand Up @@ -117,21 +118,21 @@ function getReportID(route) {
}

function ReportScreen({
betas,
route,
report,
reportActions,
accountManagerReportID,
personalDetails,
policies,
translate,
network,
isSmallScreenWidth,
isSidebarLoaded,
viewportOffsetTop,
isComposerFullSize,
errors,
}) {
betas,
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix styling, why all that spacing?

route,
report,
reportActions,
accountManagerReportID,
personalDetails,
policies,
translate,
network,
isSmallScreenWidth,
isSidebarLoaded,
viewportOffsetTop,
isComposerFullSize,
errors,
}) {
const firstRenderRef = useRef(true);
const flatListRef = useRef();
const reactionListRef = useRef();
Expand All @@ -141,7 +142,10 @@ function ReportScreen({
const [isBannerVisible, setIsBannerVisible] = useState(true);

const reportID = getReportID(route);
const {addWorkspaceRoomOrChatPendingAction, addWorkspaceRoomOrChatErrors} = ReportUtils.getReportOfflinePendingActionAndErrors(report);
const {
addWorkspaceRoomOrChatPendingAction,
addWorkspaceRoomOrChatErrors
} = ReportUtils.getReportOfflinePendingActionAndErrors(report);
const screenWrapperStyle = [styles.appContent, styles.flex1, {marginTop: viewportOffsetTop}];

// There are no reportActions at all to display and we are still in the process of loading the next set of actions.
Expand Down Expand Up @@ -291,98 +295,101 @@ function ReportScreen({
reactionListRef,
}}
>
<ScreenWrapper
style={screenWrapperStyle}
shouldEnableKeyboardAvoidingView={isTopMostReportId}
>
<FullPageNotFoundView
shouldShow={(!report.reportID && !report.isLoadingReportActions && !isLoading) || shouldHideReport}
subtitleKey="notFound.noAccess"
shouldShowCloseButton={false}
shouldShowBackButton={isSmallScreenWidth}
onBackButtonPress={Navigation.goBack}
shouldShowLink={false}
<ReportActionListFrozenScrollContextProvider>
<ScreenWrapper
style={screenWrapperStyle}
shouldEnableKeyboardAvoidingView={isTopMostReportId}
>
<OfflineWithFeedback
pendingAction={addWorkspaceRoomOrChatPendingAction}
errors={addWorkspaceRoomOrChatErrors}
shouldShowErrorMessages={false}
needsOffscreenAlphaCompositing
<FullPageNotFoundView
shouldShow={(!report.reportID && !report.isLoadingReportActions && !isLoading) || shouldHideReport}
subtitleKey="notFound.noAccess"
shouldShowCloseButton={false}
shouldShowBackButton={isSmallScreenWidth}
onBackButtonPress={Navigation.goBack}
shouldShowLink={false}
>
{headerView}
{ReportUtils.isTaskReport(report) && isSmallScreenWidth && ReportUtils.isOpenTaskReport(report) && (
<View style={[styles.borderBottom]}>
<View style={[styles.appBG, styles.pl0]}>
<View style={[styles.ph5, styles.pb3]}>
<TaskHeaderActionButton report={report} />
<OfflineWithFeedback
pendingAction={addWorkspaceRoomOrChatPendingAction}
errors={addWorkspaceRoomOrChatErrors}
shouldShowErrorMessages={false}
needsOffscreenAlphaCompositing
>
{headerView}
{ReportUtils.isTaskReport(report) && isSmallScreenWidth && ReportUtils.isOpenTaskReport(report) && (
<View style={[styles.borderBottom]}>
<View style={[styles.appBG, styles.pl0]}>
<View style={[styles.ph5, styles.pb3]}>
<TaskHeaderActionButton report={report}/>
</View>
</View>
</View>
</View>
)}
</OfflineWithFeedback>
{Boolean(accountManagerReportID) && ReportUtils.isConciergeChatReport(report) && isBannerVisible && (
<Banner
containerStyles={[styles.mh4, styles.mt4, styles.p4, styles.bgDark]}
textStyles={[styles.colorReversed]}
text={translate('reportActionsView.chatWithAccountManager')}
onClose={dismissBanner}
onPress={chatWithAccountManager}
shouldShowCloseButton
/>
)}
<DragAndDropProvider isDisabled={!isReportReadyForDisplay}>
<View
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]}
onLayout={(event) => {
// Rounding this value for comparison because they can look like this: 411.9999694824219
const newSkeletonViewContainerHeight = Math.round(event.nativeEvent.layout.height);

// The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it
// takes up so we can set the skeleton view container height.
if (newSkeletonViewContainerHeight === 0) {
return;
}
setSkeletonViewContainerHeight(newSkeletonViewContainerHeight);
}}
>
{isReportReadyForDisplay && !isLoadingInitialReportActions && !isLoading && (
<ReportActionsView
reportActions={reportActions}
report={report}
isComposerFullSize={isComposerFullSize}
parentViewHeight={skeletonViewContainerHeight}
policy={policy}
/>
)}

{/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then
we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */}
{(!isReportReadyForDisplay || isLoadingInitialReportActions || isLoading) && <ReportActionsSkeletonView containerHeight={skeletonViewContainerHeight} />}

{isReportReadyForDisplay && (
<>
<ReportFooter
pendingAction={addWorkspaceRoomOrChatPendingAction}
isOffline={network.isOffline}
</OfflineWithFeedback>
{Boolean(accountManagerReportID) && ReportUtils.isConciergeChatReport(report) && isBannerVisible && (
<Banner
containerStyles={[styles.mh4, styles.mt4, styles.p4, styles.bgDark]}
textStyles={[styles.colorReversed]}
text={translate('reportActionsView.chatWithAccountManager')}
onClose={dismissBanner}
onPress={chatWithAccountManager}
shouldShowCloseButton
/>
)}
<DragAndDropProvider isDisabled={!isReportReadyForDisplay}>
<View
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]}
onLayout={(event) => {
// Rounding this value for comparison because they can look like this: 411.9999694824219
const newSkeletonViewContainerHeight = Math.round(event.nativeEvent.layout.height);

// The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it
// takes up so we can set the skeleton view container height.
if (newSkeletonViewContainerHeight === 0) {
return;
}
setSkeletonViewContainerHeight(newSkeletonViewContainerHeight);
}}
>
{isReportReadyForDisplay && !isLoadingInitialReportActions && !isLoading && (
<ReportActionsView
reportActions={reportActions}
report={report}
isComposerFullSize={isComposerFullSize}
onSubmitComment={onSubmitComment}
policies={policies}
parentViewHeight={skeletonViewContainerHeight}
policy={policy}
/>
</>
)}
)}

{!isReportReadyForDisplay && (
<ReportFooter
shouldDisableCompose
isOffline={network.isOffline}
/>
)}
</View>
</DragAndDropProvider>
</FullPageNotFoundView>
</ScreenWrapper>
{/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then
we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */}
{(!isReportReadyForDisplay || isLoadingInitialReportActions || isLoading) &&
<ReportActionsSkeletonView containerHeight={skeletonViewContainerHeight}/>}

{isReportReadyForDisplay && (
<>
<ReportFooter
pendingAction={addWorkspaceRoomOrChatPendingAction}
isOffline={network.isOffline}
reportActions={reportActions}
report={report}
isComposerFullSize={isComposerFullSize}
onSubmitComment={onSubmitComment}
policies={policies}
/>
</>
)}

{!isReportReadyForDisplay && (
<ReportFooter
shouldDisableCompose
isOffline={network.isOffline}
/>
)}
</View>
</DragAndDropProvider>
</FullPageNotFoundView>
</ScreenWrapper>
</ReportActionListFrozenScrollContextProvider>
</ReportScreenContext.Provider>
);
}
Expand Down
10 changes: 8 additions & 2 deletions src/pages/home/report/ReportActionItemMessageEdit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import useReportScrollManager from '../../../hooks/useReportScrollManager';
import * as EmojiPickerAction from '../../../libs/actions/EmojiPickerAction';
import focusWithDelay from '../../../libs/focusWithDelay';
import ONYXKEYS from '../../../ONYXKEYS';
import useFrozenScroll from "../../../hooks/useFrozenScroll";

const propTypes = {
/** All the data of the action */
Expand Down Expand Up @@ -111,6 +112,8 @@ function ReportActionItemMessageEdit(props) {
const isFocusedRef = useRef(false);
const insertedEmojis = useRef([]);

const {setShouldFreezeScroll} = useFrozenScroll();

useEffect(() => {
// required for keeping last state of isFocused variable
isFocusedRef.current = isFocused;
Expand Down Expand Up @@ -209,6 +212,7 @@ function ReportActionItemMessageEdit(props) {
* Delete the draft of the comment being edited. This will take the comment out of "edit mode" with the old content.
*/
const deleteDraft = useCallback(() => {
setShouldFreezeScroll(false);
debouncedSaveDraft.cancel();
Report.saveReportActionDraft(props.reportID, props.action.reportActionID, '');
ComposerActions.setShouldShowComposeInput(true);
Expand All @@ -222,7 +226,7 @@ function ReportActionItemMessageEdit(props) {
keyboardDidHideListener.remove();
});
}
}, [props.action.reportActionID, debouncedSaveDraft, props.index, props.reportID, reportScrollManager]);
}, [setShouldFreezeScroll, props.action.reportActionID, debouncedSaveDraft, props.index, props.reportID, reportScrollManager]);

/**
* Save the draft of the comment to be the new comment message. This will take the comment out of "edit mode" with
Expand Down Expand Up @@ -352,11 +356,13 @@ function ReportActionItemMessageEdit(props) {
style={[styles.textInputCompose, styles.flex1, styles.bgTransparent]}
onFocus={() => {
setIsFocused(true);
reportScrollManager.scrollToIndex({animated: true, index: props.index}, true);
setShouldFreezeScroll(true);
reportScrollManager.scrollToIndex({animated: false, index: props.index}, true);
ComposerActions.setShouldShowComposeInput(false);
}}
onBlur={(event) => {
setIsFocused(false);
setShouldFreezeScroll(false);
const relatedTargetId = lodashGet(event, 'nativeEvent.relatedTarget.id');

// Return to prevent re-render when save/cancel button is pressed which cancels the onPress event by re-rendering
Expand Down
67 changes: 67 additions & 0 deletions src/pages/home/report/ReportActionListFrozenScrollContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import React, {createContext, forwardRef, useMemo, useState} from "react";
import PropTypes from "prop-types";
import getComponentDisplayName from "../../../libs/getComponentDisplayName";


const withScrollFrozenPropTypes = {
/** flag determining if we should freeze the scroll */
shouldFreezeScroll: PropTypes.bool,

/** Function to update the state */
setShouldFreezeScroll: PropTypes.func,
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
shouldFreezeScroll: PropTypes.bool,
/** Function to update the state */
setShouldFreezeScroll: PropTypes.func,
shouldFreezeScroll: PropTypes.bool.isRequired,
/** Function to update the state */
setShouldFreezeScroll: PropTypes.func.isRequired,

};

const ReportActionListFrozenScrollContext = createContext(null);


function ReportActionListFrozenScrollContextProvider(props) {
const [shouldFreezeScroll, setShouldFreezeScroll] = useState(false);

/**
* The context this component exposes to child components
* @returns {Object} flag and a flag setter
*/
const contextValue = useMemo(
() => ({
shouldFreezeScroll,
setShouldFreezeScroll,
}),
[shouldFreezeScroll, setShouldFreezeScroll],
);

return <ReportActionListFrozenScrollContext.Provider
value={contextValue}>{props.children}</ReportActionListFrozenScrollContext.Provider>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The useMemo looks like an overhead here.
cc @fabioh8010 @gedu

Suggested change
value={contextValue}>{props.children}</ReportActionListFrozenScrollContext.Provider>;
value={{
shouldFreezeScroll,
setShouldFreezeScroll,
}}>{props.children}</ReportActionListFrozenScrollContext.Provider>;

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 it's correct @rezkiy37, because if we pass an object in this way, is like we are creating a new object every time and it will trigger unnecessary re-renders (old object will never equal newly created one). It makes sense to me to memoize.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is ok to memo. it is a common practice to always memo the objects from Context

Copy link
Contributor

@ArekChr ArekChr Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This does not need to be memoized. We could return the useStateobject to the provider (as an array, not object), which will be the same reference on every render. WDYT?

function ReportActionListFrozenScrollContextProvider(props) {
    const shouldFreezeScrollState = useState(false);
    return (
        <ReportActionListFrozenScrollContext.Provider value={shouldFreezeScrollState}>
            {props.children}
        </ReportActionListFrozenScrollContext.Provider>
    );
}

Copy link
Contributor

@rezkiy37 rezkiy37 Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree with @ArekChr's approach.

}

ReportActionListFrozenScrollContextProvider.displayName = 'ReportActionListFrozenScrollContextProvider';
ReportActionListFrozenScrollContextProvider.propTypes = {
/** Actual content wrapped by this component */
children: PropTypes.node.isRequired,
};

function withScrollFrozen(WrappedComponent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we creating this HOC? It's not being used anywhere.

const WithScrollFrozenState = forwardRef((props, ref) => (
<ReportActionListFrozenScrollContext.Consumer>
{(scrollFrozenProps) =>
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
{...scrollFrozenProps}
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
ref={ref}
/>
}
</ReportActionListFrozenScrollContext.Consumer>
));

WithScrollFrozenState.displayName = `WithScrollFrozenState(${getComponentDisplayName(WrappedComponent)})`;
return WithScrollFrozenState;
}


export {
ReportActionListFrozenScrollContext,
ReportActionListFrozenScrollContextProvider,
withScrollFrozenPropTypes,
withScrollFrozen
};
Loading