Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Task Title Validation on Main Composer Text Change #52941
Add Task Title Validation on Main Composer Text Change #52941
Changes from 10 commits
9c825da
bd3a70e
bee1a66
faf4370
bcbde08
0e5325d
f90ce91
a3bd4bf
3d40a79
dd65326
cb37156
2c3b86d
708adbf
0c20987
5c51ae8
ed1b95d
867f77b
c3c0796
58bbcbe
36253a8
41c3215
3f08b65
dc57914
ad2272d
e638474
e63f27a
36e4c3e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused an issue
We should use
EMAIL_WITH_OPTIONAL_DOMAIN.source
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name it more specific to task title? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoangzinh In my opinion using a separate variable like
isCreatingTaskComment
would be redundant as we already perform regex matching inuseHandleExceedMaxTaskTitleLength
. If we implement leading true, it should be safe. But let me know if we still need to do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can move regrex matching out of useHandleExceedMaxTaskTitleLength hook. And add another wrapper debounce for validation methods like this. I think it works better. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoangzinh after test it might looks like this
src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx
src/hooks/useHandleExceedMaxTaskTitleLength.ts
proceed with the above change? or alternatively we can keep current
useHandleExceedMaxTaskTitleLength
logic but no need to debounce it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right @wildan-m . Hmm, what do you think if we refactor it a bit:
So in main-composer, we will use 1500 as a debounce time for both plain function validations above. And in edit composer, we will have another debounce with 1500ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoangzinh if we're going to extract the debounce and not need faster debounce specific for task title, how about keeping the regex matching inside the hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can @wildan-m, but it's kind of wrong in a case, let's say
validateTaskTitleMaxLength
returns false. It also means it's a task creating comment, however, the length is still in capacity. But then we have to use another validation ofvalidateCommentMaxLength
, which is unnecessary and incorrect because it's a task creating comment but we use a standard max length validation on it. Kind of double validations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option we can consider refactoring existing
validateCommentMaxLength
by passing max_length.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoangzinh the code has been updated. I use and modify
COMMENT_LENGTH_DEBOUNCE_TIME
since it is not being used anywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use nested if/else to read code easier here. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoangzinh Although the lint check will pass, I believe we may prefer an early return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer an early return if it's only 1 if. For example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a debounce function, can we trust it to return true/false value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.geeksforgeeks.org/lodash-_-debounce-method/
We set leading true, it will immediately executed at the initial attempt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I missed this comment. Hmm. I don't think it's a good idea to check conditions based on result of a debounce function. It's better if we detect comment format and use properly validation function here. For example:
what do you think?