-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Enhance SignupForm with Debounced Validation #3345
Conversation
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.
Thanks so much for your work on this!
I think this feels pretty good so far—the only changes I'd make are to remove the comments and to add back in the aria-label
property for the input field. Let me know if you have any questions or further thoughts on this!
Everything is good to go now. |
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.
Thanks for the update on this!
Just wanted to clarify that my earlier note to remove the comments was referring to all of ones within the file, not just the one that was pinpointed. Although I feel that it might be best to remove them, if you'd like we can keep the ones here within this file. In general, I think we should only include comments as an additional note or to provide further context that might not be easy to tell when looking through a file. Down to discuss this further though if you have any other thoughts on this!
I'm so sorry, I'm updating this note (which previous said this was approved) because I got confused with another one of your PRs 😭 For this one, I wanted to add that although you did add back in one of the aria properties, there's still a few that are missing from the file. I'll pinpoint the ones that need to be added back in!
Sorry again for the confusion!
Sorry I accidentally confused this with another one of your pull requests!
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.
Sorry again about the confusion!
I added in the additional places where the aria attributes should be added back in. Thanks again and let me know if you have any questions!
@raclim First of all, that was funny! Also, I agree with you. Give me a sec—I'm removing all the unnecessary comments now. |
@raclim now Can you please review it again without any confusion : ) |
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.
Thanks so much for your understanding and for the quick update!
I added in one last aria-hidden
label that was missing, but otherwise I think this feels good to me! Thanks so much again for your work on this!
Fixes #issue-3235
Changes:
Debounced Username & Email Validation: Ensures API calls are made only after 300ms of inactivity, reducing the frequency of requests during typing.
adding a debounce method which make any function or promise delay as per the require time and than applied these to async function for it.
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123