-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Support Full Name as Record Text Identifier #11610
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
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 PR adds support for using full name fields as record text identifiers, allowing them to be clickable and selectable in the settings form.
- Added
FieldMetadataType.FULL_NAME
toLABEL_IDENTIFIER_FIELD_METADATA_TYPES
array in/packages/twenty-front/src/modules/object-metadata/constants/LabelIdentifierFieldMetadataTypes.ts
- Removed 'fullName' from reserved keywords in
/packages/twenty-server/src/engine/metadata-modules/utils/validate-metadata-name-is-not-reserved-keyword.ts
- Enhanced
RecordChip
component in/packages/twenty-front/src/modules/object-record/components/RecordChip.tsx
to handle full name concatenation - Modified
ChipFieldDisplay
anduseChipFieldDisplay
to properly handle full name fields as identifiers - Updated
SettingsDataModelObjectIdentifiersForm
to keep identifier fields clickable even with single field
💡 (5/5) You can turn off certain types of comments like style here!
6 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
let name = recordChipData.name; | ||
|
||
if (field?.type === FieldMetadataType.FULL_NAME) { | ||
name = `${record[field.name]?.firstName || ''} ${record[field.name]?.lastName || ''}`; |
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.
style: Consider adding .trim() to remove extra space when either firstName or lastName is empty
name = `${record[field.name]?.firstName || ''} ${record[field.name]?.lastName || ''}`; | |
name = `${record[field.name]?.firstName || ''} ${record[field.name]?.lastName || ''}`.trim(); |
'currencies', | ||
'fullName', | ||
'fullNames', |
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.
style: Consider whether 'fullNames' should also be removed for consistency since 'fullName' is now allowed
packages/twenty-front/src/modules/object-record/components/RecordChip.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/object-record/components/RecordChip.tsx
Outdated
Show resolved
Hide resolved
...y-server/src/engine/metadata-modules/utils/validate-metadata-name-is-not-reserved-keyword.ts
Show resolved
Hide resolved
fullWidth | ||
dropdownId={`${fieldName}-select`} | ||
emptyOption={emptyOption} | ||
options={options} | ||
value={value} | ||
withSearchInput={label === t`Record label`} |
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 don't understand this change
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.
In Issue, required change is only in 'Record Label' dropdown, Not in 'Record Image' dropdown.
...ules/settings/data-model/objects/forms/components/SettingsDataModelObjectIdentifiersForm.tsx
Show resolved
Hide resolved
@@ -22,6 +23,10 @@ export const ChipFieldDisplay = () => { | |||
size={ChipSize.Small} | |||
to={labelIdentifierLink} | |||
isLabelHidden={isLabelIdentifierCompact} | |||
field={{ |
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.
see comment above
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.
Hi @b9aurav, thanks for the PR
I have left comments :)
@charlesBochet Thanks for your comments, I'll take a look at requested changes. |
@@ -13,7 +13,7 @@ export const getLabelIdentifierFieldValue = ( | |||
objectNameSingular === CoreObjectNameSingular.WorkspaceMember || | |||
labelIdentifierFieldMetadataItem?.type === FieldMetadataType.FULL_NAME | |||
) { | |||
return `${record.name?.firstName ?? ''} ${record.name?.lastName ?? ''}`; | |||
return `${record[labelIdentifierFieldMetadataItem?.name ?? '']?.firstName ?? ''} ${record[labelIdentifierFieldMetadataItem?.name ?? '']?.lastName ?? ''}`; |
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 should throw if we can't find the labelIdentifierFieldMetadataItem
@@ -20,19 +22,26 @@ export const useRecordChipData = ({ | |||
PreComputedChipGeneratorsContext, | |||
); | |||
|
|||
const { fieldDefinition } = useContext(FieldContext); | |||
const updatedRecord = { ...record }; |
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 avoid creating a fake record having a "name", this will create confusion in the code. Instead, we can passe to identifierChipGenerator and generateDefaultRecordChipData what we need
Thank you @b9aurav, I've made some small modifications to remove previous technical debt |
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:31034 This environment will automatically shut down when the PR is closed or after 5 hours. |
Thanks @b9aurav for your contribution! |
closes #11296
recording.webm