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

Add default Record representation #8011

Merged
merged 23 commits into from
Jul 28, 2022
Merged

Add default Record representation #8011

merged 23 commits into from
Jul 28, 2022

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Jul 26, 2022

Problem

React-admin forces developers to define a child <Field> for <ReferenceField> to represent the related record. This is cumbersome.

Solution

Add the ability to define a recordRepresentation at the <Resource> level, and let <ReferenceField>. Use that knowledge to render without a child.

-<ReferenceField source="user_id" reference="users">
-    <TextField source="name" />
-</ReferenceField>
+<ReferenceField source="user_id" reference="users" />
  • Update <Resource> and ResourceDefinitionContext to carry the recordRepresentation
  • Add a useGetRecordRepresentation hook
  • Use recordRepresentation in <ReferenceField> and let it work without children
  • Use recordRepresentation in <ReferenceOneField> and let it work without children
  • Use recordRepresentation in <Edit> and <Show> page titles
  • Use recordRepresentation in <SelectInput> and let it work without optionText when used in <ReferenceInput>
  • Use recordRepresentation in <AutocompleteInput> and let it work without optionText when used in <ReferenceInput>
  • Use recordRepresentation in <ReferenceInput> and let it work without children
  • Update <ListGuesser>, <EditGuesser> and <ShowGuesser> to output child-less References
  • Update tutorial
  • Add doc (and rewrite the <SelectInput>, <AutocompleteInput>, and <ReferenceInput> docs in the process)
  • Add tests

Closes #4899

@fzaninotto fzaninotto added the WIP Work In Progress label Jul 26, 2022
docs/Resource.md Outdated Show resolved Hide resolved
docs/Resource.md Outdated Show resolved Hide resolved
examples/simple/src/comments/index.tsx Outdated Show resolved Hide resolved
docs/Tutorial.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine Fricker <102964006+septentrion-730n@users.noreply.github.com>
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so good! 💪

docs/ReferenceField.md Outdated Show resolved Hide resolved
packages/ra-core/src/core/useGetRecordRepresentation.ts Outdated Show resolved Hide resolved
@@ -47,12 +47,12 @@ const frenchMessages: TranslationMessages = {
page: {
create: 'Créer %{name}',
dashboard: 'Tableau de bord',
edit: '%{name} #%{id}',
edit: '%{name} %{recordRepresentation}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change for all translation packages

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not. Existing translations will still work as before. We still pass the id, the recordRepresentation is passed in addition to other message args. The new translation takes advantage of it.

@@ -123,10 +115,11 @@ ReferenceInput.defaultProps = {
page: 1,
perPage: 25,
sort: { field: 'id', order: 'DESC' },
children: <AutocompleteInput />,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't a good opportunity to avoid defaultProps? We could initialize those defaults when destructuring the props

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice to have

fzaninotto and others added 2 commits July 28, 2022 10:26
Co-authored-by: Gildas Garcia <1122076+djhi@users.noreply.github.com>
@davidhenley
Copy link
Contributor

davidhenley commented Aug 30, 2022

This is great! Thank you!

@davidhenley
Copy link
Contributor

davidhenley commented Aug 30, 2022

@djhi @fzaninotto So if we want to validate the AutocompleteInput to be required can we do this?

<ReferenceInput source="territoryId" reference="territories" validate={[required()]} />

instead of

<ReferenceInput source="territoryId" reference="territories">
  <AutocompleteInput validate={[required()]}/>
</ReferenceInput>

Is there anything wrong with passing the props (label, validate, etc) down from ReferenceInput to AutocompleteInput? That would REALLY cut the code down!

@slax57
Copy link
Contributor

slax57 commented Aug 31, 2022

@davidhenley currently you can't.
<ReferenceInput> default child is simply <AutocompleteInput />, so if you need to add other props, you have to declare the child explicitly.

I was about to answer that having the <AutocompleteInput> props also available on <ReferenceInput> might be confusing for the users, but actually they already share all the common input props. Only props that are not (yet) available on <ReferenceInput> are those specific to <AutocompleteInput>: debounce, filterToQuery, inputText, setFilter, shouldRenderSuggestions and TextFieldProps.

So yeah, actually, it's defendable, and does not look too complicated to do imho.
@djhi @fzaninotto what do you think?

@fzaninotto
Copy link
Member Author

Personally, I never know if I have to pass the label or the validate prop to the ReferenceInput of its child. It's true that this part of the API is confusing.

But:

  • some ReferenceInput do support a label (e.g. RefrenceArrayInput put its own label)
  • Some ReferenceInput support the validation themselves, not their children (e.g. ReferenceArrayInput, again)

I'm not sure we can draw systematically pass props down.

I'll need to play with a Storybook that does all possibilities to understand the best decision to make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants