Add Select form field #8815
Add Select form field #8815
Conversation
|
|
||
| {draftValue.editingMode === 'edit' ? ( | ||
| <SelectableList | ||
| selectableListId={SINGLE_RECORD_SELECT_BASE_LIST} |
There was a problem hiding this comment.
Not sure about the id to use here.
| }: FormSelectFieldInputProps) => { | ||
| const inputId = useId(); | ||
|
|
||
| const hotkeyScope = InlineCellHotkeyScope.InlineCell; |
There was a problem hiding this comment.
We might want to create another scope
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a new select form field component and integrates it into the existing form system, with support for variable picking and keyboard navigation.
- Added new
FormSelectFieldInput.tsxcomponent with hotkey scope management and variable support - Modified
FormFieldInput.tsxto handle select fields usingisFieldSelectguard - Updated
WorkflowEditActionFormRecordCreate.tsxto support metadata-driven select fields - Added type safety improvements by replacing
FieldMetadataItemwithFieldDefinition<FieldMetadata> - Implemented keyboard navigation and filtering in select field component following existing patterns
3 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -23,7 +26,6 @@ export const FormFieldInput = ({ | |||
| }: FormFieldInputProps) => { | |||
| return isFieldNumber(field) ? ( | |||
| <FormNumberFieldInput | |||
| key={field.id} | |||
| label={field.label} | |||
| defaultValue={defaultValue as string | number | undefined} | |||
There was a problem hiding this comment.
logic: type assertion could be unsafe here - consider adding runtime type checking before the cast
| onPersist={onPersist} | ||
| field={field} | ||
| VariablePicker={VariablePicker} | ||
| /> | ||
| ) : null; |
There was a problem hiding this comment.
style: consider throwing an error for unhandled field types instead of silently returning null
...-front/src/modules/object-record/record-field/form-types/components/FormSelectFieldInput.tsx
Outdated
Show resolved
Hide resolved
| const onCancel = () => { | ||
| if (draftValue.type !== 'static') { | ||
| throw new Error('Can only be called when editing a static value'); | ||
| } |
There was a problem hiding this comment.
style: Consider handling this error case more gracefully instead of throwing - could lead to runtime crashes if state gets corrupted
| const optionIds = [ | ||
| `No ${field.label}`, | ||
| ...filteredOptions.map((option) => option.value), | ||
| ]; |
There was a problem hiding this comment.
logic: optionIds includes 'No {field.label}' but this ID isn't handled in handleSelectEnter - could cause undefined behavior
packages/twenty-front/src/modules/workflow/components/WorkflowEditActionFormRecordCreate.tsx
Outdated
Show resolved
Hide resolved
| <Tag | ||
| preventShrink | ||
| color={selectedOption.color} | ||
| text={selectedOption.label} | ||
| /> |
There was a problem hiding this comment.
can you use <SelectFieldDisplay component here?
There was a problem hiding this comment.
I've considered using the existing FieldInput components; however, they rely on the FieldContext. See the useSelectField and useSelectFieldDisplay hooks. We don't currently provide this context. If we do, we will couple the FormFieldInputs with the usual FieldInputs. This might be considered, but it is an opinionated choice.
I duplicated most of SelectFieldInput's code. We might reconsider this and find a way to reduce the code.
There was a problem hiding this comment.
we might create a new components to decouple the component design definition from the fieldContext. Thus we will be able to factorize
There was a problem hiding this comment.
I created a component to unify what we display for the display mode of a select input.
There was a problem hiding this comment.
nice, is it possible to do the same with the input mode?
There was a problem hiding this comment.
Would you like to extract this part?
<SelectableList
selectableListId={SINGLE_RECORD_SELECT_BASE_LIST}
selectableItemIdArray={optionIds}
hotkeyScope={hotkeyScope}
onEnter={(itemId) => {
const option = filteredOptions.find(
(option) => option.value === itemId,
);
if (isDefined(option)) {
onSubmit?.(() => persistField(option.value));
resetSelectedItem();
}
}}
>
<SelectInput
parentRef={selectWrapperRef}
onOptionSelected={handleSubmit}
options={fieldDefinition.metadata.options}
onCancel={onCancel}
defaultOption={selectedOption}
onFilterChange={setFilteredOptions}
onClear={
fieldDefinition.metadata.isNullable ? handleClearField : undefined
}
clearLabel={fieldDefinition.label}
hotkeyScope={hotkeyScope}
/>
</SelectableList>I'm not sure it would be great, as I would end up with a component taking +10 unrelated props.
There was a problem hiding this comment.
For me, the SelectableList and SelectInput components are well enough abstracted. They own the design decisions. These two components are meta components with business logic. I don't think we need to go deeper into the refactoring.
There was a problem hiding this comment.
I think yes you should extract this part into packages/twenty-front/src/modules/ui/field/input/components in a SelectInput component and use that in FormSelectFieldInput and in SelectFieldInput
There was a problem hiding this comment.
Like this, the way to display or modify a SELECT value is uniquely defined throughout the application
| </StyledDisplayModeContainer> | ||
|
|
||
| {draftValue.editingMode === 'edit' ? ( | ||
| <SelectableList |
There was a problem hiding this comment.
same here, can you use <SelectFieldInput component?
There was a problem hiding this comment.
The same reasoning applies here: #8815 (comment)
| const availableFieldMetadataItems = objectMetadataItem.fields | ||
| .filter( | ||
| (fieldMetadataItem) => | ||
| fieldMetadataItem.type !== FieldMetadataType.Relation && | ||
| !fieldMetadataItem.isSystem && | ||
| fieldMetadataItem.isActive, | ||
| ) | ||
| .sort((fieldMetadataItemA, fieldMetadataItemB) => |
There was a problem hiding this comment.
@thomtrp, we should not forget to do that also for Update Action when developed
Devessier
force-pushed
the
add-select-form-field
branch
from
664c2b7 to
943dca6
Compare
| import { Tag, ThemeColor } from 'twenty-ui'; | ||
|
|
||
| type SelectFieldDisplayContentProps = { | ||
| color: ThemeColor; | ||
| label: string; | ||
| }; | ||
|
|
||
| export const SelectFieldDisplayContent = ({ | ||
| color, | ||
| label, | ||
| }: SelectFieldDisplayContentProps) => { | ||
| return <Tag preventShrink color={color} text={label} />; | ||
| }; |
There was a problem hiding this comment.
think you should move that to packages/twenty-front/src/modules/ui/field/display/components and call it SelectDisplay
…et complete definitions
…de the next draft value we set in the clearField function
Devessier
force-pushed
the
add-select-form-field
branch
from
278d2f1 to
09f15d0
Compare
|
Thanks @Devessier for your contribution! |
Closes twentyhq#8815 I took inspiration from existing parts of the codebase. Please, see the comments I left below. Remaining questions: - I'm not sure about the best way to handle hotkey scopes in the components easily https://github.com/user-attachments/assets/7a6dd144-d528-4f68-97cd-c9181f3954f9
Closes #8815
I took inspiration from existing parts of the codebase. Please, see the comments I left below.
Remaining questions:
CleanShot.2024-12-02.at.17.35.16.mp4