Migrate ExceededCommentLength to functional component - #19676
Conversation
|
@mountiny @rushatgabhane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
@rushatgabhane all yours!
| const [commentLength, setCommentLength] = useState(0); | ||
| const updateCommentLength = useMemo( | ||
| () => | ||
| debounce((comment, onExceededMaxCommentLength) => { |
There was a problem hiding this comment.
@szebniok We don't have a style guide yet, but I prefer that we use useCallback to memoize functions (because it's consistent with our codebase)
React.useCallback(function helloWorld(){}, []);
// ...Is functionally equivalent to :
React.useMemo(() => function helloWorld(){}, []);There was a problem hiding this comment.
In this case we are not returning the function straight away, but passing it to the debounce function.
Since the useCallback is not using a function as it's first argument, the debounce function will be evaluated on each re-render (so basically, each time the text props changes), but the result will always be ignored, since the dependency array is empty.
Is the debounce function too expensive to call on each re-render? I would argue that it is a lot of code to be run on each keystroke, considering that we didn't do it before.
There was a problem hiding this comment.
I agree with @szebniok here, although I need to refresh my hooks knowledge too
There was a problem hiding this comment.
Thank you for the well explained comment!
|
|
||
| useEffect(() => { | ||
| updateCommentLength(props.comment, props.onExceededMaxCommentLength); | ||
| }, [props.comment, props.onExceededMaxCommentLength, updateCommentLength]); |
There was a problem hiding this comment.
Why are we calling updateCommentLength() if updateCommentLength has changed in dependency array.
Could you please help me understand this
There was a problem hiding this comment.
Or to repharse my question, why does updateCommentLength need to be part of the dependency array?
There was a problem hiding this comment.
Without adding the updateCommentLength to the dependency array, the eslint is throwing the react-hooks/exhaustive-deps warning. It is a bit annoying, as this function will never change (it is cached by useMemo).
I could add a eslint-disable-next-line with an additional comment, but the current solution seems simpler and less error-prone in the future.
|
@rushatgabhane Would you be able to give this another look? |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-05-29.at.22.12.42.movMobile Web - ChromeScreen.Recording.2023-05-29.at.22.17.04.movMobile Web - SafariScreen.Recording.2023-05-29.at.22.13.58.movDesktopScreen.Recording.2023-05-29.at.22.14.25.moviOSScreen.Recording.2023-05-29.at.22.18.15.movAndroidScreen.Recording.2023-05-29.at.22.19.41.mov |
|
✋ 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/mountiny in version: 1.3.21-0 🚀
|
|
Works in staging as outlined |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.21-2 🚀
|
5 participants
Details
Fixed Issues
$ #16149
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)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)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
Web
Had small issue with the focus when editing the message while recording.
desktop.mp4
Mobile Web - Chrome
Screen.Recording.2023-05-26.at.17.36.27.mov
Mobile Web - Safari
safari.mp4
Desktop
electron.mp4
iOS
ios.mp4
Android
android.mp4