Fix/30921: No error message when editing a chat#31769
Conversation
| return; | ||
| } | ||
| setExceededMaxCommentLength(true); | ||
| }, [value, setExceededMaxCommentLength, hasExceededMaxCommentLength]); |
There was a problem hiding this comment.
I don't like this - I think we should rename these two props:
setExceededMaxCommentLength to onMaxLengthExceeded,
hasExceededMaxCommentLengthto maxLengthExceeded
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@burczu Updated based on your suggestion
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2023-12-08.at.10.18.22.movAndroid: mWeb ChromeScreen.Recording.2023-12-08.at.10.26.39.moviOS: NativeScreen.Recording.2023-12-08.at.10.20.21.moviOS: mWeb SafariScreen.Recording.2023-12-08.at.10.22.59.movMacOS: Chrome / SafariScreen.Recording.2023-12-08.at.10.37.00.movMacOS: DesktopScreen.Recording.2023-12-08.at.10.30.31.mov |
|
@DylanDylann I've found some warning when I open a chat with a very long message - please see screenshots below:
Looks like it's related with the prop you've added, so please fix it. |
|
Also, please check the below video of Native Android, there is some strange behavior: Screen.Recording.2023-11-29.at.10.46.07.movWhen I delete the word P.S. Checked also on web, and it does not happen there, but there is another problem - when the text has almost 10k characters, there is significant delay between typing the letters on the keyboard and appearing them in the input (this issue disappears right after the limit is exceeded and the error message appears), so I guess |
|
@burczu In the above, you mentioned:
So you mean debounce when update the |
|
@DylanDylann Yeah, I think so but please test it. |
|
@burczu Sure, will test it |
I cannot reproduce this one. If can, please help give me the steps to reproduce Screencast.from.30-11-2023.16.40.03.webm |
@burczu |
rather than |
|
@DylanDylann Regarding this:
I've tested again and wasn't able to reproduce it too, so maybe it's fixed now - I'll let you know if I encounter it again. When it comes to this one:
Can we try to tackle this issue - I guess it is related to the code we touch while working on this issue. Regarding the last comment, yes - I think we should give the solution you've proposed a try. |
|
I am working on this comment #31769 (comment) |
|
There was a problem hiding this comment.
LGTM in general.
The only concern I have is that we have code duplication in ComposerWithSuggestion and ReportActionItemMessageEdit components (handleValueChange and handleValueChangeDebounce methods are the same in both places) - maybe we could think about extracting them to some shared custom hook? But it does not seem to be widely used approach in the App... I wonder what the Expensify engineer that will be assigned thinks about it.
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
I agree I would like to see if the duplicated logic for handleValueChange could be cleaned up and shared. I think a custom hook is fine.
|
@burczu @tgolen I just created the hook based on this comment #31769 (review) |
|
|
||
| const handleValueChangeDebounce = useMemo(() => _.debounce(handleValueChange, 1500), [handleValueChange]); | ||
|
|
||
| return {hasExceededMaxCommentLength, handleValueChangeDebounce}; |
There was a problem hiding this comment.
Could you please rename handleValueChangeDebounce to validateCommentMaxLength? There are a few reasons for this:
- Functions should be named for what they do, not what they handle
- It isn't necessary for anything to know that it's debounced (it's an implementation detail)
- It is a bit ambiguous without mentioning the word "comment"
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
Looks really good. Thank you!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 1.4.13-0 🚀
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 1.4.13-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.13-8 🚀
|
4 participants
Details
Fixed Issues
$ #30921
PROPOSAL: #30921 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screencast.from.23-11-2023.19.42.54.webm
Android: mWeb Chrome
Screencast.from.23-11-2023.10.58.09.webm
iOS: Native
Screencast.from.23-11-2023.19.52.33.webm
iOS: mWeb Safari
Screencast.from.23-11-2023.11.01.59.webm
MacOS: Chrome / Safari
Screencast.from.23-11-2023.10.56.54.webm
MacOS: Desktop
Screencast.from.23-11-2023.18.51.13.webm