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

Fix/30921: No error message when editing a chat #31769

Merged
merged 9 commits into from
Dec 13, 2023
Merged

Fix/30921: No error message when editing a chat #31769

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
56ae3df
fix no error message when editing a chat
DylanDylann Nov 23, 2023
9344c1e
fix refactor the onValueChange
DylanDylann Nov 28, 2023
61ae94b
fix merge main
DylanDylann Nov 30, 2023
a6fb968
fix merge main
DylanDylann Dec 6, 2023
dd81868
fix perf issue
DylanDylann Dec 6, 2023
8909420
fix create useHandleExceedMaxCommentLength
DylanDylann Dec 12, 2023
8063878
fix add underscore lib
DylanDylann Dec 12, 2023
62718fc
fix add lodash lib
DylanDylann Dec 12, 2023
37acb42
fix rename function
DylanDylann Dec 13, 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
44 changes: 5 additions & 39 deletions src/components/ExceededCommentLength.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,23 @@
import {debounce} from 'lodash';
import PropTypes from 'prop-types';
import React, {useEffect, useMemo, useState} from 'react';
import {withOnyx} from 'react-native-onyx';
import React from 'react';
import useLocalize from '@hooks/useLocalize';
import * as ReportUtils from '@libs/ReportUtils';
import useThemeStyles from '@styles/useThemeStyles';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import Text from './Text';

const propTypes = {
/** Report ID to get the comment from (used in withOnyx) */
// eslint-disable-next-line react/no-unused-prop-types
reportID: PropTypes.string.isRequired,

/** Text Comment */
comment: PropTypes.string,

/** Update UI on parent when comment length is exceeded */
onExceededMaxCommentLength: PropTypes.func.isRequired,
shouldShowError: PropTypes.bool.isRequired,
};

const defaultProps = {
comment: '',
};
const defaultProps = {};

function ExceededCommentLength(props) {
const styles = useThemeStyles();
const {numberFormat, translate} = useLocalize();
const [commentLength, setCommentLength] = useState(0);
const updateCommentLength = useMemo(
() =>
debounce((comment, onExceededMaxCommentLength) => {
const newCommentLength = ReportUtils.getCommentLength(comment);
setCommentLength(newCommentLength);
onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH);
}, CONST.TIMING.COMMENT_LENGTH_DEBOUNCE_TIME),
[],
);

useEffect(() => {
updateCommentLength(props.comment, props.onExceededMaxCommentLength);
}, [props.comment, props.onExceededMaxCommentLength, updateCommentLength]);

if (commentLength <= CONST.MAX_COMMENT_LENGTH) {
if (!props.shouldShowError) {
return null;
}

return (
<Text
style={[styles.textMicro, styles.textDanger, styles.chatItemComposeSecondaryRow, styles.mlAuto, styles.pl2]}
Expand All @@ -61,9 +32,4 @@ ExceededCommentLength.propTypes = propTypes;
ExceededCommentLength.defaultProps = defaultProps;
ExceededCommentLength.displayName = 'ExceededCommentLength';

export default withOnyx({
comment: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
initialValue: '',
},
})(ExceededCommentLength);
export default ExceededCommentLength;
12 changes: 12 additions & 0 deletions src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ function ComposerWithSuggestions({
forwardedRef,
isNextModalWillOpenRef,
editFocused,
setExceededMaxCommentLength,
hasExceededMaxCommentLength,
// For testing
children,
}) {
Expand Down Expand Up @@ -526,6 +528,16 @@ function ComposerWithSuggestions({
[blur, focus, prepareCommentAndResetComposer, replaceSelectionWithText],
);

useEffect(() => {
if (ReportUtils.getCommentLength(value) <= CONST.MAX_COMMENT_LENGTH) {
if (hasExceededMaxCommentLength) {
setExceededMaxCommentLength(false);
}
return;
}
setExceededMaxCommentLength(true);
}, [value, setExceededMaxCommentLength, hasExceededMaxCommentLength]);
Copy link
Contributor

@burczu burczu Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like this - I think we should rename these two props:

setExceededMaxCommentLength to onMaxLengthExceeded,
hasExceededMaxCommentLengthto maxLengthExceeded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@burczu I think we do not need to rename these two props, because we also call setExceededMaxCommentLength(false) when the input `s length < 10K. So it does not make sense if we call onMaxLengthExceeded(false)

Copy link
Contributor

@burczu burczu Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think, to keep the separation of concerns, the responsibility for setting the state should be moved to the component that holds this state, so here, in this file we should have:

useEffect(() => {
  onValueChange(value);
}, [value]);

and in the ReportActionCompose we should handle onValueChange (onValueChange={handleValueChange}) like below:

handleValueChange = (value) => {
  if (ReportUtils.getCommentLength(value) <= CONST.MAX_COMMENT_LENGTH) {
    if (hasExceededMaxCommentLength) {
        setExceededMaxCommentLength(false);
    }
    return;
  }
  setExceededMaxCommentLength(true);
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@burczu Updated based on your suggestion


return (
<>
<View style={[containerComposeStyles, styles.textInputComposeBorder]}>
Expand Down
7 changes: 3 additions & 4 deletions src/pages/home/report/ReportActionCompose/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,8 @@ function ReportActionCompose({
onBlur={onBlur}
measureParentContainer={measureContainer}
listHeight={listHeight}
setExceededMaxCommentLength={setExceededMaxCommentLength}
hasExceededMaxCommentLength={hasExceededMaxCommentLength}
/>
<ReportDropUI
onDrop={(e) => {
Expand Down Expand Up @@ -452,10 +454,7 @@ function ReportActionCompose({
>
{!isSmallScreenWidth && <OfflineIndicator containerStyles={[styles.chatItemComposeSecondaryRow]} />}
<ReportTypingIndicator reportID={reportID} />
<ExceededCommentLength
reportID={reportID}
onExceededMaxCommentLength={setExceededMaxCommentLength}
/>
<ExceededCommentLength shouldShowError={hasExceededMaxCommentLength} />
</View>
</OfflineWithFeedback>
</View>
Expand Down
16 changes: 11 additions & 5 deletions src/pages/home/report/ReportActionItemMessageEdit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,16 @@ function ReportActionItemMessageEdit(props) {
*/
const focus = focusComposerWithDelay(textInputRef.current);

useEffect(() => {
if (ReportUtils.getCommentLength(draft) <= CONST.MAX_COMMENT_LENGTH) {
if (hasExceededMaxCommentLength) {
setHasExceededMaxCommentLength(false);
}
return;
}
setHasExceededMaxCommentLength(true);
}, [draft, hasExceededMaxCommentLength]);

return (
<>
<View style={[styles.chatItemMessage, styles.flexRow]}>
Expand Down Expand Up @@ -466,11 +476,7 @@ function ReportActionItemMessageEdit(props) {
</View>
</View>
</View>
<ExceededCommentLength
comment={draft}
reportID={props.reportID}
onExceededMaxCommentLength={(hasExceeded) => setHasExceededMaxCommentLength(hasExceeded)}
/>
<ExceededCommentLength shouldShowError={hasExceededMaxCommentLength} />
</>
);
}
Expand Down
Loading