-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Remove deprecated EMAIL, PHONE, LINK #7551
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.
PR Summary
This pull request implements significant changes to the field metadata types and related components in the Twenty application, focusing on simplifying and consolidating field types. Here are the key modifications:
- Removed EMAIL, LINK, and PHONE from FieldMetadataType enum in GraphQL schema
- Eliminated standalone EmailFieldDisplay, LinkFieldDisplay, and PhoneFieldDisplay components
- Removed corresponding field input components and their Storybook stories
- Consolidated email, link, and phone field types into more detailed structures (e.g., emails, links, phones)
- Updated usePersistField hook to remove checks for email, link, and phone fields
- Modified FieldInput and FieldDisplay components to reflect the new field type structure
These changes aim to improve the application's structure and potentially fix issues with composite filters, but may require careful review to ensure existing functionality is not broken.
29 file(s) reviewed, 20 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -27,13 +26,10 @@ import { ChipFieldDisplay } from '../meta-types/display/components/ChipFieldDisp | |||
import { CurrencyFieldDisplay } from '../meta-types/display/components/CurrencyFieldDisplay'; | |||
import { DateFieldDisplay } from '../meta-types/display/components/DateFieldDisplay'; | |||
import { DateTimeFieldDisplay } from '../meta-types/display/components/DateTimeFieldDisplay'; | |||
import { EmailFieldDisplay } from '../meta-types/display/components/EmailFieldDisplay'; | |||
import { FullNameFieldDisplay } from '../meta-types/display/components/FullNameFieldDisplay'; |
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.
logic: EmailFieldDisplay, LinkFieldDisplay, and PhoneFieldDisplay components were removed. Verify if they're replaced by other components or if this removal is intentional.
import { isFieldPhones } from '@/object-record/record-field/types/guards/isFieldPhones'; | ||
import { isFieldRawJson } from '@/object-record/record-field/types/guards/isFieldRawJson'; | ||
import { isFieldRelationFromManyObjects } from '@/object-record/record-field/types/guards/isFieldRelationFromManyObjects'; | ||
import { isFieldRelationToOneObject } from '@/object-record/record-field/types/guards/isFieldRelationToOneObject'; |
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.
logic: The removal of isFieldDisplayedAsPhone might affect how phone fields are rendered. Ensure this change doesn't break existing functionality.
) : isFieldPhones(fieldDefinition) ? ( | ||
<PhonesFieldInput onCancel={onCancel} /> |
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.
logic: Phone field input now only handles isFieldPhones, not individual phone fields. Verify this doesn't cause issues with single phone number inputs.
) : isFieldEmails(fieldDefinition) ? ( | ||
<EmailsFieldInput onCancel={onCancel} /> |
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.
logic: Email field input now only handles isFieldEmails, not individual email fields. Confirm this change doesn't affect single email input functionality.
) : isFieldLinks(fieldDefinition) ? ( | ||
<LinksFieldInput onCancel={onCancel} /> |
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.
logic: Link field input now only handles isFieldLinks, not individual link fields. Ensure this doesn't break single link input functionality.
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.
logic: The entire useLinkField hook has been removed. This will likely cause issues for components that depend on this 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.
logic: Entire file deleted. Ensure all components using usePhoneField are 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.
logic: The entire usePhoneFieldDisplay hook has been removed. This will likely cause issues in components that depend on this hook, such as PhoneFieldDisplay.
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.
logic: The entire LinkFieldInput component has been removed. Ensure this functionality is implemented elsewhere or that it's no longer needed.
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.
logic: Removing this file eliminates link field value validation. Ensure this doesn't break type safety elsewhere in the codebase.
case FieldMetadataType.Emails: | ||
return 'EMAILS'; | ||
case FieldMetadataType.Phone: | ||
return 'PHONE'; |
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 did not remove LINK above here
FieldMetadataType.DateTime, | ||
FieldMetadataType.Date, | ||
FieldMetadataType.Email, |
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.
Link bellow in that file
In this PR: - remove deprecated EMAIL, PHONE, LINK field types (except for Zapier package as there is another work ongoing) - remove composite currency filter on currencyCode, actor filter on name and workspaceMember as the UX is not great yet
In this PR: