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

Fix error validation on email address field #725

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mohsenkhosroanjam
Copy link

@mohsenkhosroanjam mohsenkhosroanjam commented Feb 9, 2025

User description

Description

When the user enters an invalid email address, they will get an error "Please enter a valid email address"<br** />

Fixes #695

Dependencies

I used toast from sonner

Mentions

Mention and tag the people

Screenshots of relevant screens

Screenshot 2025-02-09 at 2 36 48 PM

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • 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

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Bug fix, Enhancement


Description

  • Fixed email validation to prevent browser tooltip errors.

  • Updated email input type to text for consistent validation.

  • Added onSubmit prop to InputBorderSpotlight for better form handling.

  • Simplified onChange handler for email input field.


Changes walkthrough 📝

Relevant files
Enhancement
index.tsx
Updated form handling in Hero component                                   

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

  • Added onSubmit prop to InputBorderSpotlight component.
  • Simplified EncryptButton onClick handler.
  • +2/-5     
    Bug fix
    input-spotlight.tsx
    Enhanced email input handling and validation                         

    apps/web/src/components/ui/input-spotlight.tsx

  • Added onSubmit prop for form submission handling.
  • Changed email input type from email to text.
  • Simplified onChange handler for email input.
  • +5/-5     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 9, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 844b042)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    695 - Partially compliant

    Compliant requirements:

    • Remove browser tooltip validation error for invalid email addresses

    Non-compliant requirements:

    • Show consistent "Please enter a valid email address" error message

    Requires further human verification:

    • Verify that the error message "Please enter a valid email address" appears correctly for both empty and invalid email cases
    • Test the behavior across different browsers to ensure consistent validation
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Email Validation

    Changing input type from 'email' to 'text' removes browser validation but custom email validation logic is not implemented to show the required error message

    type="text"
    Event Handler

    The onClick handler for EncryptButton is incorrectly passing the onSubmit function reference instead of calling it

    <EncryptButton TARGET_TEXT="Join Waitlist" onClick={() => onSubmit} />

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 9, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 844b042

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect event handler invocation

    The onClick handler for EncryptButton is incorrectly passing a function
    reference instead of calling it. Change onClick={() => onSubmit} to onClick={()
    => onSubmit()} to properly invoke the submit handler.

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

    -<EncryptButton TARGET_TEXT="Join Waitlist" onClick={() => onSubmit} />
    +<EncryptButton TARGET_TEXT="Join Waitlist" onClick={() => onSubmit()} />
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The current code passes the function reference without invoking it, which would prevent form submission. This fix is critical for the waitlist button functionality.

    High
    General
    Restore proper input validation type

    For proper email validation, the input type should be 'email' instead of 'text'
    to leverage browser's built-in email format validation and appropriate mobile
    keyboard.

    apps/web/src/components/ui/input-spotlight.tsx [62]

    -type="text"
    +type="email"
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Using type="email" provides built-in email validation and better mobile UX. This is important for data quality and user experience.

    Medium
    • More

    Previous suggestions

    Suggestions up to commit 844b042
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect event handler invocation

    The onClick handler for EncryptButton is incorrectly passing the onSubmit
    function reference instead of calling it. Change the arrow function to properly
    invoke onSubmit.

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

    -<EncryptButton TARGET_TEXT="Join Waitlist" onClick={() => onSubmit} />
    +<EncryptButton TARGET_TEXT="Join Waitlist" onClick={() => onSubmit()} />
    Suggestion importance[1-10]: 9

    __

    Why: The current code passes the function reference without invoking it, which would prevent form submission. This is a critical bug fix that would restore core functionality.

    High
    General
    Restore proper input field validation

    Change input type back to 'email' to leverage browser's built-in email
    validation and provide better mobile keyboard layout for email entry.

    apps/web/src/components/ui/input-spotlight.tsx [62]

    -type="text"
    +type="email"
    Suggestion importance[1-10]: 7

    __

    Why: Using type="email" provides built-in email validation and better mobile UX. This is an important usability improvement that helps prevent invalid email submissions.

    Medium
    ✅ Suggestions up to commit 05eb73b
    CategorySuggestion                                                                                                                                    Impact
    General
    Enhance input field accessibility

    Keep the input type as 'email' for better accessibility and native browser
    validation, while maintaining custom validation logic.

    apps/web/src/components/ui/input-spotlight.tsx [59-76]

     <input
       autoCapitalize="off"
       onFocus={handleFocus}
       onMouseEnter={handleMouseEnter}
       onMouseLeave={handleMouseLeave}
       onMouseMove={handleMouseMove}
       placeholder="Enter your email address"
       size={25}
    -  type="text"
    +  type="email"
    +  inputMode="email"
     />
    Suggestion importance[1-10]: 9

    __

    Why: Reverting to type="email" and adding inputMode="email" significantly improves accessibility and user experience by leveraging built-in browser validation while maintaining custom validation. This is a crucial enhancement for form usability.

    High
    Possible issue
    Improve email validation error handling
    Suggestion Impact:The commit removed the email validation function entirely, which is a different approach than the suggestion but addresses the same concern of improving email validation handling

    code diff:

    -  const validateEmail = (email: string): void => {
    -    const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/
    -    if (!emailRegex.test(email)){
    -      toast.custom(() => (
    -        <div className="text-brandBlue border-brandBlue/20 bg-errorRed w-[90vw] rounded-lg border p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]">
    -        <p className="text-sm">Please enter a valid email address</p>
    -        </div>
    -      ))
    -      return
    -    }
    -    }

    Add error state handling to prevent form submission with invalid email and
    provide immediate user feedback by validating email on input change instead of a
    separate function.

    apps/web/src/components/ui/input-spotlight.tsx [45-55]

    -const validateEmail = (email: string): void => {
    +const validateEmail = (email: string): boolean => {
       const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/
    -  if (!emailRegex.test(email)){
    +  const isValid = emailRegex.test(email)
    +  if (!isValid) {
         toast.custom(() => (
           <div className="text-brandBlue border-brandBlue/20 bg-errorRed w-[90vw] rounded-lg border p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]">
    -      <p className="text-sm">Please enter a valid email address</p>
    +        <p className="text-sm">Please enter a valid email address</p>
           </div>
         ))
    -    return
       }
    -  }
    +  return isValid
    +}

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion improves error handling by returning a boolean value and making the validation function more reusable. The improved code also fixes indentation issues and provides clearer control flow.

    Medium

    @@ -59,7 +72,7 @@ export function InputBorderSpotlight({
    onMouseMove={handleMouseMove}
    placeholder="Enter your email address"
    size={25}
    type="email"
    type="text"
    Copy link
    Member

    Choose a reason for hiding this comment

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

    We don't want to change this

    Copy link
    Author

    @mohsenkhosroanjam mohsenkhosroanjam Feb 9, 2025

    Choose a reason for hiding this comment

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

    I was wondering how the tooltip could be removed. As I know, the tooltip error is thrown from the browser and when the type is "email", the browser does the validation and throws error with tooltip. Am I right?
    I have changed it to text to let the browser know that I don't want the browser to check this field whether it is email or not.

    Copy link
    Member

    Choose a reason for hiding this comment

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

    @kriptonian1 what are your thoughts on this? I think the browser tooltip looks out of theme, and it should be replaced with the toast

    Copy link

    @mohsenkhosroanjam, please resolve all open reviews!

    @github-actions github-actions bot closed this Feb 23, 2025
    Copy link

    PR closed due to inactivity

    Copy link

    PR closed due to inactivity

    @github-actions github-actions bot closed this Feb 23, 2025
    @rajdip-b rajdip-b reopened this Feb 23, 2025
    Copy link

    @mohsenkhosroanjam, please resolve all open reviews; otherwise this PR will be closed after Sun Feb 16 2025 11:07:36 GMT+0000 (Coordinated Universal Time)!

    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.

    Email validation error
    2 participants