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.

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
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
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

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;

}
168 changes: 86 additions & 82 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import DragAndDropProvider from '../../components/DragAndDrop/Provider';
import usePrevious from '../../hooks/usePrevious';
import CONST from '../../CONST';
import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../../components/withCurrentReportID';
import {ReportActionListFrozenScrollContextProvider} from "./report/ReportActionListFrozenScrollContext";

const propTypes = {
/** Navigation route context info provided by react navigation */
Expand Down Expand Up @@ -357,98 +358,101 @@ function ReportScreen({
reactionListRef,
}}
>
<ScreenWrapper
style={screenWrapperStyle}
shouldEnableKeyboardAvoidingView={isTopMostReportId}
testID={ReportScreen.displayName}
>
<FullPageNotFoundView
shouldShow={shouldShowNotFoundPage}
subtitleKey="notFound.noAccess"
shouldShowCloseButton={false}
shouldShowBackButton={isSmallScreenWidth}
shouldShowLink={false}
<ReportActionListFrozenScrollContextProvider>
<ScreenWrapper
style={screenWrapperStyle}
shouldEnableKeyboardAvoidingView={isTopMostReportId}
testID={ReportScreen.displayName}
>
<OfflineWithFeedback
pendingAction={addWorkspaceRoomOrChatPendingAction}
errors={addWorkspaceRoomOrChatErrors}
shouldShowErrorMessages={false}
needsOffscreenAlphaCompositing
<FullPageNotFoundView
shouldShow={shouldShowNotFoundPage}
subtitleKey="notFound.noAccess"
shouldShowCloseButton={false}
shouldShowBackButton={isSmallScreenWidth}
shouldShowLink={false}
>
{headerView}
{ReportUtils.isTaskReport(report) && isSmallScreenWidth && ReportUtils.isOpenTaskReport(report, parentReportAction) && (
<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, parentReportAction) && (
<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={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={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={isOffline}
reportActions={reportActions}
report={report}
isComposerFullSize={isComposerFullSize}
onSubmitComment={onSubmitComment}
policies={policies}
/>
</>
)}

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

const propTypes = {
/** All the data of the action */
Expand Down Expand Up @@ -122,6 +123,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 @@ -235,6 +238,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, '');

Expand All @@ -250,7 +254,7 @@ function ReportActionItemMessageEdit(props) {
keyboardDidHideListener.remove();
});
}
}, [props.action.reportActionID, debouncedSaveDraft, props.index, props.reportID, reportScrollManager, isActive]);
}, [setShouldFreezeScroll, props.action.reportActionID, debouncedSaveDraft, props.index, props.reportID, reportScrollManager, isActive]);

/**
* 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 @@ -382,8 +386,9 @@ function ReportActionItemMessageEdit(props) {
maxLines={isSmallScreenWidth ? CONST.COMPOSER.MAX_LINES_SMALL_SCREEN : CONST.COMPOSER.MAX_LINES} // This is the same that slack has
style={[styles.textInputCompose, styles.flex1, styles.bgTransparent]}
onFocus={() => {
setIsFocused(true);
reportScrollManager.scrollToIndex({animated: true, index: props.index}, true);
setIsFocused(true);
setShouldFreezeScroll(true);
setShouldShowComposeInputKeyboardAware(false);

// Clear active report action when another action gets focused
Expand All @@ -396,6 +401,7 @@ function ReportActionItemMessageEdit(props) {
}}
onBlur={(event) => {
setIsFocused(false);
setShouldFreezeScroll(false);
const relatedTargetId = lodashGet(event, 'nativeEvent.relatedTarget.id');
if (_.contains([messageEditInput, emojiButtonID], relatedTargetId)) {
return;
Expand Down
67 changes: 67 additions & 0 deletions src/pages/home/report/ReportActionListFrozenScrollContext.js
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

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

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
};
7 changes: 7 additions & 0 deletions src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import reportPropTypes from '../../reportPropTypes';
import FloatingMessageCounter from './FloatingMessageCounter';
import ReportActionsListItemRenderer from './ReportActionsListItemRenderer';
import reportActionPropTypes from './reportActionPropTypes';
import useFrozenScroll from "../../../hooks/useFrozenScroll";

const propTypes = {
/** The report currently being looked at */
Expand Down Expand Up @@ -90,6 +91,10 @@ function keyExtractor(item) {
return item.reportActionID;
}

const maintainVisibleContentPositionOptions = {
minIndexForVisible: 1,
}

function isMessageUnread(message, lastReadTime) {
return Boolean(message && lastReadTime && message.created && lastReadTime < message.created);
}
Expand Down Expand Up @@ -130,6 +135,7 @@ function ReportActionsList({
opacity.value = withTiming(1, {duration: 100});
}, [opacity]);
const [skeletonViewHeight, setSkeletonViewHeight] = useState(0);
const {shouldFreezeScroll} = useFrozenScroll();

useEffect(() => {
// If the reportID changes, we reset the userActiveSince to null, we need to do it because
Expand Down Expand Up @@ -360,6 +366,7 @@ function ReportActionsList({
}}
onScroll={trackVerticalScrolling}
extraData={extraData}
maintainVisibleContentPosition={shouldFreezeScroll ? maintainVisibleContentPositionOptions : null}
/>
</Animated.View>
</>
Expand Down
Loading