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.

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

chore: Email validation fixes #474

Closed
wants to merge 0 commits into from
Closed

chore: Email validation fixes #474

wants to merge 0 commits into from

Conversation

bansal-harsh-2504
Copy link
Contributor

@bansal-harsh-2504 bansal-harsh-2504 commented Oct 2, 2024

User description

Description

There was a bug where if we type email as xyz@xyz it was getting accepted but now i have implemented validation using regex now it email should look like [email protected].

Fixes #463

Dependencies

Future Improvements

Mentions

@rajdip-b @kriptonian1

##Screenshots
Old Behaviour
old

Current Behaviour
Screenshot 2024-10-02 225232

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work
  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

not needed


PR Type

enhancement, bug fix


Description

  • Implemented email validation using a regular expression to ensure the email format is correct.
  • Fixed a typo in the error message for when the email input is empty.
  • Added a new error message for invalid email formats to improve user feedback.

Changes walkthrough 📝

Relevant files
Enhancement
index.tsx
Implement email validation and fix typo in error message 

apps/web/src/components/hero/index.tsx

  • Added email validation using regex.
  • Fixed typo in the error message for empty email input.
  • Displayed error message for invalid email format.
  • +13/-2   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible UX Issue
    The error messages for empty email and invalid email format use the same styling, which might confuse users. Consider differentiating them visually.

    Code Duplication
    The toast.custom() call is repeated twice with similar content. Consider extracting this into a reusable function to improve maintainability.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Oct 2, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract email validation logic into a separate function for improved readability and reusability

    Extract the email validation logic into a separate function to improve code
    readability and reusability. This will also make it easier to test the validation
    logic independently.

    apps/web/src/components/hero/index.tsx [16-34]

    -const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/
    +const validateEmail = (email: string): string | null => {
    +  if (email === '') {
    +    return 'Please enter an email address';
    +  }
    +  const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
    +  if (!emailRegex.test(email)) {
    +    return 'Please enter a valid email address';
    +  }
    +  return null;
    +}
     
    -if (email === '') {
    +const errorMessage = validateEmail(email);
    +if (errorMessage) {
       toast.custom(() => (
         <div className="text-brandBlue border-brandBlue/20 w-[90vw] rounded-lg border bg-[#852b2c] p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]">
    -      <p className="text-sm">Please enter an email address </p>
    +      <p className="text-sm">{errorMessage}</p>
         </div>
       ))
    -  return
    +  return;
     }
     
    -if (!emailRegex.test(email)) {
    -  toast.custom(() => (
    -    <div className="text-brandBlue border-brandBlue/20 w-[90vw] rounded-lg border bg-[#852b2c] p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]">
    -      <p className="text-sm">Please enter a valid email address</p>
    -    </div>
    -  ))
    -  return
    -}
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Extracting the email validation logic into a separate function enhances code readability and reusability, making it easier to maintain and test the validation logic independently.

    8
    Enhancement
    ✅ Improve email validation with a more comprehensive regex pattern
    Suggestion Impact:The commit updated the email validation regex to a more comprehensive pattern, similar to the suggestion, by changing the domain part to allow word characters and enforcing a minimum of two characters for the top-level domain.

    code diff:

    +    const emailRegex = /^[^\s@]+@[^\s@]+\.[\w]{2,}$/
    +
    +    if (email === '' || !emailRegex.test(email)) {

    Consider using a more robust email validation regex. The current regex
    /^[^\s@]+@[^\s@]+.[^\s@]+$/ is simple but may allow some invalid email formats. A
    more comprehensive regex can provide better validation while still being relatively
    simple.

    apps/web/src/components/hero/index.tsx [16]

    -const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/
    +const emailRegex = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a more comprehensive regex pattern for email validation is valid and can improve the accuracy of the validation, reducing the chances of accepting invalid email formats.

    7
    Best practice
    Use a constant for repeated toast message styling to improve maintainability

    Consider using a constant for the toast message styling to avoid repetition and
    improve maintainability. This will make it easier to update the styling in the
    future if needed.

    apps/web/src/components/hero/index.tsx [20]

    -<div className="text-brandBlue border-brandBlue/20 w-[90vw] rounded-lg border bg-[#852b2c] p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]">
    +const TOAST_STYLE = "text-brandBlue border-brandBlue/20 w-[90vw] rounded-lg border bg-[#852b2c] p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]";
     
    +// ... later in the code ...
    +<div className={TOAST_STYLE}>
    +
    Suggestion importance[1-10]: 6

    Why: Using a constant for the repeated toast message styling is a good practice that improves maintainability by reducing repetition and making future updates to the styling easier.

    6

    💡 Need additional feedback ? start a PR chat

    @bansal-harsh-2504 bansal-harsh-2504 changed the title #463 chore: email validation chore: email validation fixes(#463) Oct 2, 2024
    @rajdip-b rajdip-b changed the title chore: email validation fixes(#463) chore: Email validation fixes(#463) Oct 3, 2024
    @rajdip-b rajdip-b changed the title chore: Email validation fixes(#463) chore: Email validation fixes Oct 3, 2024
    apps/web/src/components/hero/index.tsx Outdated Show resolved Hide resolved
    @bansal-harsh-2504
    Copy link
    Contributor Author

    @rajdip-b Can you please add hacktoberfest label?

    @@ -13,10 +13,12 @@ function Hero(): React.JSX.Element {
    const onSubmit = (e: React.FormEvent): void => {
    e.preventDefault()

    if (email === '') {
    const emailRegex = /^[^\s@]+@[^\s@]+\.[\w]{2,}$/
    Copy link
    Member

    Choose a reason for hiding this comment

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

    image
    The pattern is still broken. Please fix it. Use regex101 to test your patterns.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    ([a-zA-Z0-9]+)([_.-{1}])?([a-zA-Z0-9]+)@([a-zA-Z0-9]+)([.])([a-zA-Z.]+)
    @rajdip-b
    I think this regex will do but it is failing pre commit checks because of es-lint, it says
    "Capture group '([a-zA-Z0-9]+)' should be converted to a named or non-capturing group."
    but if i change it to name capturing then it says
    "Named capturing groups are only available when targeting 'ES2018' or later."

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    @bansal-harsh-2504 you can just use email validation from zod, it will make your life easy

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Oct 8, 2024

    @bansal-harsh-2504 any particular reason why you have closed this? the pr looks good

    @bansal-harsh-2504
    Copy link
    Contributor Author

    @rajdip-b Everytime i commit my changes somehow 50-60 files get modified on their own, so i have to delete the directory and clone the fork again after each commit and now after last commit it was showing that fork is 2 commits behind and 4 commits ahead , so i did not know what to do. I deleted the pr and fork, now i will create another pr and hopefully solve this issue.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Oct 8, 2024

    hmm, thats odd. You can use --no-verify flag when you commit. that will prevent the changes

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    WEB: suggesting changes while adding waitlist
    3 participants