-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add address composite form field #9022
Conversation
thomtrp
commented
Dec 11, 2024
- Create FormCountrySelectInput using the existing FormSelectFieldInput
- Create AddressFormFieldInput component
- Fix FormSelectFieldInput dropdown + add leftIcon
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 comprehensive address form field functionality with country selection support. Here's a concise summary of the key changes:
- Created new
FormAddressFieldInput
component with fields for street address, city, state, postal code and country - Added
FormCountrySelectInput
with flag icons and country selection dropdown - Modified
FormSelectFieldInput
to support icons and transparent backgrounds for country selection - Updated
Tag
andMenuItemSelectTag
components to handle country flag icons and transparent backgrounds - Potential issue: Using countryName instead of countryCode in
FormCountrySelectInput
could cause internationalization problems
The implementation appears solid overall but should consider using countryCode as the value identifier for better data consistency and internationalization support.
14 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings | Greptile
...ront/src/modules/object-record/record-field/form-types/components/FormCountrySelectInput.tsx
Show resolved
Hide resolved
icon: (props: IconComponentProps) => | ||
Flag({ width: props.size, height: props.size }), // TODO : improve this ? | ||
}), |
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: Flag component is being recreated on every render. Consider memoizing the icon function or moving it outside the map.
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.
Instead, I would create a wrapper component outside of the FormCountrySelectInput
. Here, we'll recreate the React component on every render unecessarily.
const FlagIcon = (props: IconComponentProps) => {
return Flag({ width: props.size, height: props.size })
}
// ...
icon: FlagIcon,
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.
Flag comes from useCountries. It cannot be extracted. I cannot memoize either because this is inside a function
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.
Understandable. I didn't see where the Flag function was coming from!
...ront/src/modules/object-record/record-field/form-types/components/FormCountrySelectInput.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/object-record/record-field/components/FormFieldInput.tsx
Show resolved
Hide resolved
...front/src/modules/object-record/record-field/form-types/components/FormAddressFieldInput.tsx
Outdated
Show resolved
Hide resolved
...ect-record/record-field/form-types/components/__stories__/FormCountrySelectInput.stories.tsx
Show resolved
Hide resolved
...ject-record/record-field/form-types/components/__stories__/FormAddressFieldInput.stories.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/field/display/components/SelectDisplay.tsx
Show resolved
Hide resolved
...s/twenty-front/src/modules/ui/input/components/internal/country/components/CountrySelect.tsx
Show resolved
Hide resolved
packages/twenty-ui/src/navigation/menu-item/components/MenuItemSelectTag.tsx
Show resolved
Hide resolved
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.
Great work! Works well. We can merge.
options={field.metadata.options} | ||
clearLabel={field.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.
Dissociating the field definition from the input is excellent. I'm unsure if it will "scale" well as the other fields make extensive use of the useNumberField
-like functions and get access to the field definition that way.
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 see. If it becomes too complex we will still be able to come back to full field!
...front/src/modules/object-record/record-field/form-types/components/FormAddressFieldInput.tsx
Outdated
Show resolved
Hide resolved
icon: (props: IconComponentProps) => | ||
Flag({ width: props.size, height: props.size }), // TODO : improve this ? | ||
}), |
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.
Instead, I would create a wrapper component outside of the FormCountrySelectInput
. Here, we'll recreate the React component on every render unecessarily.
const FlagIcon = (props: IconComponentProps) => {
return Flag({ width: props.size, height: props.size })
}
// ...
icon: FlagIcon,
...ront/src/modules/object-record/record-field/form-types/components/FormCountrySelectInput.tsx
Outdated
Show resolved
Hide resolved
} | ||
|
||
if (countryCode === null) { | ||
onPersist(''); |
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 know much about addresses, but are we sure we want to store an empty string vs. null
?
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.
yep default values have to be string