-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Add RecordField component #10749
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
base: next
Are you sure you want to change the base?
Add RecordField component #10749
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than that LGTM
/> | ||
``` | ||
|
||
The `source`, `field`, `children`, and `render` props are mutually exclusive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true. You can have source
and field
for instance.
Did you mean the following?
The `source`, `field`, `children`, and `render` props are mutually exclusive. | |
The `field`, `children`, and `render` props are mutually exclusive. |
<RecordField source="title" empty="resources.books.fields.title.missing" /> | ||
``` | ||
|
||
If you use the `render` prop, you can even use a React element as `empty` value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes as a surprise to me. Why require a render
prop to support a React element? To me, you should be able to use a React element in all cases. You can simply render empty
(when it's a React element) instead of the provided children
/field
/render
when the value is empty.
[`& .${RecordFieldClasses.label}`]: { | ||
fontSize: '0.75em', | ||
marginBottom: '0.2em', | ||
color: (theme.vars || theme).palette.text.secondary, | ||
}, | ||
[`&.${RecordFieldClasses.inline} .${RecordFieldClasses.label}`]: { | ||
fontSize: '0.875rem', | ||
display: 'block', | ||
minWidth: 150, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FMI, why are you using em
units with the default variant, but rem
units for the inline variant?
interface ComponentNameToClassKey { | ||
RaRecordField: 'root' | 'content' | 'button' | 'icon'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface ComponentNameToClassKey { | |
RaRecordField: 'root' | 'content' | 'button' | 'icon'; | |
} | |
interface ComponentNameToClassKey { | |
RaRecordField: 'root' | 'label' | 'value' | 'inline'; | |
} |
Problem
<SimpleShowLayout>
uses child inspection to decorate its children with labels.This isn't robust, as you just need to wrap a child component to break this decoration:
Solution
Invert the responsibilities : it's no longer the layout's role to add a label, but the field's.
Provide a new component to render a field and its label,
RecordField
:How To Test
Use the fields/RecordField story
Additional Checks
master
for a bugfix or a documentation fix, ornext
for a feature