record board card creation using pending record id #9236
Conversation
| ); | ||
|
|
||
| return ( | ||
| <FieldInput |
There was a problem hiding this comment.
not sure if we want this or inlineCells to be opened in edit mode.
|
@ehconitin I've tried to test the behavior but fixing the conflicts is not completely straightforward. Could you fix them first? Thank you! |
|
Taking another look |
|
Starting with the functional review:
|
| import { RecordBoardContext } from '../contexts/RecordBoardContext'; | ||
| import { recordBoardPendingRecordIdByColumnComponentFamilyState } from '../states/recordBoardPendingRecordIdByColumnComponentFamilyState'; | ||
|
|
||
| export const useUpsertBoardOpportunityRecord = (recordBoardId: string) => { |
There was a problem hiding this comment.
This looks duplicated with useUpsertBoardRecord below. I'm not fully against it if it's hard to merge them (I would prefer two clear hooks over one which is hard to maintin) but I feel we could try to merge them.
Long term, we plan to address record creation through the RightDrawer so I would not over invest on this area. The abstract reason why for "opportunity" we need to fill the "company" is that "company" would be a REQUIRED field on opportunity. Right now it's hard coded for opportunity and it's fine keeping it like this as long as we haven't introduced both required relations and creation in Right Drawer.
So I would not over engineer this and why not just making company optional in useUpsertBoardRecord for now and merge both hooks
| ); | ||
|
|
||
| const [pendingRecord] = useRecoilState( | ||
| recordBoardPendingRecordIdByColumnState(columnId), |
There was a problem hiding this comment.
this should be a componentState
| }} | ||
| > | ||
| <RecordInlineCellEditMode> | ||
| <RecordBoardCellFieldInput /> |
| import { useContext } from 'react'; | ||
| import { useRecoilCallback } from 'recoil'; | ||
|
|
||
| export const RecordBoardCellFieldInput = () => { |
There was a problem hiding this comment.
I would name this component differently. I feel it's RecordBoardPendingTextIdentifierFieldInput
|
|
||
| const handleEnter: FieldInputEvent = (persistField) => { | ||
| persistField(); | ||
| onUpsertRecord({ |
There was a problem hiding this comment.
shouldn't we call useUpsertBoardRecord here?
Looks like the main challenge:
- if we wouldn't have the opporunity (+ company case), we only upsert the record once the name is filled (so calling useUpsertBoardRecord here), I think that's what's done on the RecordTable
- but as we need to persist the company too, you are persisting the record sooner and only updating it here now.
I would try to call useUpsertBoardRecord (so no persist it sooner), and instead for the opportunity.company case, I would write in the recordStore { recordId: pendingRecordId, company: xxx, position: xxx } in useCreateNewBoardRecord
It should work!
| @@ -166,7 +154,9 @@ export const RecordBoardColumnHeader = () => { | |||
| <LightIconButton | |||
| accent="tertiary" | |||
| Icon={IconPlus} | |||
| onClick={() => handleNewButtonClick('first', isOpportunity)} | |||
| onClick={() => | |||
| createNewBoardRecord(columnDefinition.id, 'first') | |||
| onCancel={() => createNewBoardRecord(columnId, position)} | ||
| onRecordSelected={(company) => { | ||
| if (isDefined(company)) { | ||
| upsertBoardOpportunityRecord(columnId, company); |
There was a problem hiding this comment.
so I would not upsert here but createNewBoardRecord + write in recordStore instead.
|
@ehconitin I'm closing the PR to keep the history clean, I'm happy to re-review it once ready / help on this one if needed :) |
#8941 (review)