-
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
feat: add Links field type #5176
Conversation
}, | ||
{ | ||
name: 'secondaryLinks', | ||
type: FieldMetadataType.TEXT, |
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.
should be an array?
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.
It's an array of TEXT
, the isArray
flag is set to true in the next lines :)
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.
As discussed with @charlesBochet, this property is now a RAW_JSON.
@@ -286,6 +286,7 @@ export enum FieldMetadataType { | |||
Currency = 'CURRENCY', | |||
Date = 'DATE', | |||
DateTime = 'DATE_TIME', | |||
Domain = 'DOMAIN', |
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.
Let's rename this to LINKS and try to do something that accommodate different type of links with the options, including domain. We could have two field type DOMAINS and LINKS but it's probably not worth it
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.
Done!
@@ -73,6 +73,7 @@ const previewableTypes = [ | |||
FieldMetadataType.Text, | |||
FieldMetadataType.Address, | |||
FieldMetadataType.RawJson, | |||
FieldMetadataType.Domain, |
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.
Let's keep it behind a feature flag maybe if additionalX is not even displayed?
The goal would be to do a perfect "primary + additionalXXX" composite field (we can start with link, then phone number, emails)
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.
We will keep both field in prod for something, then run a migration script, then deprecate PHONE, EMAIL, LINK to only keep PHONES, EMAILS, LINKS. But on those plural fields we will have a checkbox option to allow the user to disable the "additional" behavior and keep the same user experience they have with singular link/phone/email fields today. CC @Bonapara who can provide more details
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.
I added FieldMetadataType.Links
to the excludedFieldTypes
list in the "New Field" page, so users can't create 'Links' fields for now.
19ed756
to
9c06d3a
Compare
9c06d3a
to
ca8caf7
Compare
Closes twentyhq#5113 --------- Co-authored-by: Lucas Bordeau <[email protected]>
Closes #5113