Add ReferenceOneField component
#79
Conversation
|
Excellent, looking forward to seeing this complete! |
slax57
left a comment
slax57
left a comment
There was a problem hiding this comment.
Sorry we took so long to review.
There are a few minor issues, and a small confusion about what component should be responsible of rendering the label, but apart from that this looks very promising!
Thank you for your contribution!
Feel free to ask if you have further questions.
| <DateField source="published_at" /> | ||
| <ReferenceField source="authorId" reference="authors" /> | ||
| <ReferenceOneField reference="book_details" target="book_id"> | ||
| <RecordField label="Genre" source="genre" /> |
There was a problem hiding this comment.
Why use RecordField and not, say, TextField here? 🤔
This is contradictory to what this doc says at line 20.
UPDATE: reading the whole PR and notably the stories, I realize you may be confused about what component is responsible to render the label.
RecordFieldshould be used only for cases where you are not a direct children of a component responsible of rendering the labels, likeSimpleShowLayoutfor instance.- Using
ReferenceOneFieldas direct child of aSimpleShowLayoutshould render a label on top of the field, derived either from thesourceor from thelabelprop if one is provided. - Usually,
ReferenceOneFieldwill be used to render a single child, like aTextField, that's why it doesn't need to render its child component's label too. - However if you choose to render several children fields in a
ReferenceOneField, then yes it makes sense to useRecordFieldhere. And you will need to hide the label of theReferenceOneFieldby either not rendering it in aSimpleShowLayoutor by passinglabel={false}to it.
I hope it makes sense. Feel free to ask further questions if still not clear.
| | Prop | Required | Type | Default | Description | | ||
| |------|----------|------|---------|-------------| | ||
| | `source` | Required | `string` | - | Foreign key in current record | | ||
| | `reference` | Required | `string` | - | Target resource name | | ||
| | `target` | Required | `string` | - | Target field carrying the relationship on the referenced resource, e.g. `book_id` | | ||
| | `children` | Optional | `ReactNode` | `<span>` representation | Custom child (can use context hooks) | | ||
| | `empty` | Optional | `ReactNode` | - | Placeholder when no id / value | | ||
| | `error` | Optional | `ReactNode` | - | Error element (set `false` to hide) | | ||
| | `loading` | Optional | `ReactNode` | - | Element while loading (set `false` to hide) | | ||
| | `queryOptions` | Optional | `UseQueryOptions` | - | TanStack Query options (meta, staleTime, etc.) | | ||
| | `record` | Optional | `object` | Context record | Explicit record | | ||
| | `render` | Optional | `(ctx)=>ReactNode` | - | Custom renderer receiving reference field context | | ||
| | `link` | Optional | `LinkToType` | `edit` | Link target or false / function | | ||
| | `offline` | Optional | `ReactNode` | - | The text or element to display when there is no network connectivity | |
There was a problem hiding this comment.
It seems you forgot about the filter and sort props. Also, can you order the Optional props alphabetically?
| ## Record Representation | ||
|
|
||
| [See `ReferenceField`](./ReferenceField.md#record-representation) | ||
|
|
||
| ## Tips | ||
|
|
||
| [See `ReferenceField`](./ReferenceField.md#tips) No newline at end of file |
There was a problem hiding this comment.
Can you copy these sections instead of referencing them?
| export const InShowLayout = () => ( | ||
| <Wrapper dataProvider={dataProvider}> | ||
| <div className="flex flex-col gap-4"> | ||
| <TextField source="name" /> | ||
| <ReferenceOneField reference="workoutDetails" source="short_id" target="workout_id"> | ||
| <RecordField source="note" label="Workout note" /> | ||
| <RecordField source="duration" label="Duration" /> | ||
| </ReferenceOneField> | ||
| </div> | ||
| </Wrapper> | ||
| ) |
There was a problem hiding this comment.
This story should use SimpleShowLayout
| it('should render its child in the context of the related record', async () => { | ||
| const screen = render(<Basic />); | ||
| await expect.element(screen.getByText(EXPECTED_WORKOUT_NOTE)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should allow to render the referenceRecord using a render prop', async () => { | ||
| const screen = render(<WithRenderProp />); | ||
| await expect.element(screen.getByText(EXPECTED_WORKOUT_NOTE)).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Can you also add a test to cover the case where children are passed?
| "recordfield", | ||
| "referencearrayfield", | ||
| "referencefield", | ||
| "referenceonefield", |
There was a problem hiding this comment.
please sort by alphabetical order
| export const WithRenderProp = () => ( | ||
| <Wrapper dataProvider={slowDataProvider}> | ||
| <ReferenceOneField | ||
| reference="workoutDetails" | ||
| source="short_id" | ||
| target="workout_id" | ||
| render={({ isPending, error, referenceRecord }) => { | ||
| if (isPending) { | ||
| return <p>Loading...</p>; | ||
| } | ||
| if (error) { | ||
| return <p style={{ color: 'red' }}>{error.toString()}</p> | ||
| } | ||
| return (<span>{referenceRecord ? referenceRecord.note : <b>No note.</b>}</span>) | ||
| }} /> | ||
| </Wrapper> | ||
| ) |
There was a problem hiding this comment.
I never see the 'Loading...' message with this story. Can you please check the implementation?
|
Hi, we took the time to review your patch. Can you please amend it to conform with our comments? Otherwise we'll have to close the PR. |
3 participants
Closes #78 Adding missing
ReferenceOneFieldcomponentTODO before submitting PR :
Add(not needed! After some digging it looks like Shadcn RA does not uselabelproplabelon theReferenceXcomponents but gets its label from the childRecordFieldcomponent)renderpropShowlayoutemptypropemptyprop value