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: validate emails in record-fields #7245

Merged
merged 6 commits into from
Oct 3, 2024
Merged

Conversation

sid0-0
Copy link
Contributor

@sid0-0 sid0-0 commented Sep 25, 2024

fix: #7149

Introduced a minimal field validation framework for record-fields.
Currently only shows errors for email field.

image image

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request introduces email validation for record fields, addressing the issue of accepting any input as a valid email address.

  • Added validators.ts with email regex for consistent validation across components
  • Updated EmailsFieldInput.tsx to use new validation logic, improving data integrity
  • Modified MultiItemFieldInput.tsx to handle and display validation errors
  • Enhanced DropdownMenuInput.tsx with error display functionality for better user feedback
  • Implemented minimal field validation framework, focusing on email fields initially

5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings

@bosiraphael bosiraphael self-assigned this Sep 25, 2024
Copy link
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

Hello @sid0-0! Thank you for contributing. For validation in the front end we use zod. You can look at an example of validation in packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsBlocklistInput.tsx. Could you use this framework for consistency with the rest of the codebase? Thank you :)

@bosiraphael
Copy link
Contributor

Actually, I'm not even sure that we want to constraint the inputs for the emails. I'll let @Bonapara or @FelixMalfait confirm.

@FelixMalfait
Copy link
Member

@bosiraphael I think it's cool to have it on the frontend. Ideally we would also have backend driven constraints but maybe it's okay to start with that for some field types.
Thanks for the contribution @sid0-0!

@Bonapara
Copy link
Member

For V1, I think we should change the border to red instead of writing "invalid email" under the field. @sid0-0 can you manage that? Thanks for contributing!

CleanShot 2024-09-26 at 09 20 50

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=28562-75025&node-type=frame&t=P5326GhVoupZCaI8-11

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Love this improvement! Thank you for your contribution!

@FelixMalfait FelixMalfait merged commit a946c6a into twentyhq:main Oct 3, 2024
1 of 7 checks passed
Copy link

github-actions bot commented Oct 3, 2024

Thanks @sid0-0 for your contribution!
This marks your 2nd PR on the repo. You're top 18% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
fix: twentyhq#7149 

Introduced a minimal field validation framework for record-fields.
Currently only shows errors for email field.

<img width="350" alt="image"
src="https://github.com/user-attachments/assets/1a1fa790-71a4-4764-a791-9878be3274f1">
<img width="347" alt="image"
src="https://github.com/user-attachments/assets/e22d24f2-d1a7-4303-8c41-7aac3cde9ce8">

---------

Co-authored-by: sid0-0 <[email protected]>
Co-authored-by: bosiraphael <[email protected]>
Co-authored-by: Félix Malfait <[email protected]>
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.

Emails doesn't follow regex pattern in table
4 participants