Skip to content
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

Refactor Field Inputs #3658

Merged
merged 7 commits into from
Jan 27, 2024
Merged

Refactor Field Inputs #3658

merged 7 commits into from
Jan 27, 2024

Conversation

charlesBochet
Copy link
Member

@charlesBochet charlesBochet commented Jan 27, 2024

While working on the RecordBoard, I have face a few issues with Record Fields ; mainly, the logic got over complex over the last months as we have added more cases. Especially, we started maintaining an initialValue state, an editMode state, and editValue state.

I've decided to make a first step to make FieldInput and FieldDisplay behave as proper "components". It is still a bit over complex in my opinion but I have removed several states.

Here is how it works after this PR:

Record Store

We maintain a separated record-store recoil state. Ideally, we don't need to have it and we can rely on apollo instead. For now we keep it. Fields assume this record-store recoil state has been populated with the right data.
Use: useSetRecordInStore prealably to sync records from apollo in this store

FieldDisplay and FieldInput

The base of FIelds is still FieldDIsplay and FieldInput. Right now they are sharing many useFieldType hooks. I think we should go one step further and introduce useFieldTypeDisplay and useFieldTypeInput. Once done, we can fully separate FieldDisplay from FieldInput components. I also think we should complete the renaming to RecordFieldDisplay and RecordFieldInput

Field Context and FieldInputScope

Right now in the code we are still having both. We need to clarify this as it creates a coupling between TableCell, InlineCell and FieldContext. I think it is fine. Right now, FieldInput and FIeldDisplay expect FieldContext to be setup in the hiearchy

Field Draft Values vs Field Value

I am introducing a "draft value" which is very similar to the field value but accept broader user inputs as we decided to let the user a lot of freedom while typing and then validating the input on cell blur

How to use Fields (summary)

<YourComponentEffect>
const { records } = useFindManyQuery()
useSetRecordInStore(records)
</YourComponentEffect
<FieldContext>
<FieldInput> or <FieldDisplay>
</FieldDisplay>

on fieldInput opening (in tableCell or inlineCell), programatically call to populate the draftValue

  const {
    initDraftValue,
  } = useRecordFieldInput(entityId);

I think we can still improve a lot the API of FieldInput but I'll stop here for now

Copy link

github-actions bot commented Jan 27, 2024

TODOs/FIXMEs:

  • /* TODO: Displaying Tooltips on the board is causing performance issues https://react-tooltip.com/docs/examples/render */}: packages/twenty-front/src/modules/object-record/record-inline-cell/components/RecordInlineCellContainer.tsx

Generated by 🚫 dangerJS against cf47a04

@charlesBochet charlesBochet changed the title Rename field to record-field folder Refactor Field Inputs Jan 27, 2024
@charlesBochet charlesBochet merged commit ada8f55 into main Jan 27, 2024
13 checks passed
@charlesBochet charlesBochet deleted the make-field-component branch January 27, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant