-
Notifications
You must be signed in to change notification settings - Fork 8
Add readonly field #3798
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: main
Are you sure you want to change the base?
Add readonly field #3798
Conversation
7775251
to
24ed73a
Compare
24ed73a
to
70acc04
Compare
I'd like to challenge the need for
|
}; | ||
|
||
const Wrapper = createComponentSlot("div")({ | ||
componentName: "ReadOnlyBlockField", |
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.
@jamesricky these components are themable but not exported in src/index.ts
. Does this work?
}; | ||
|
||
const Label = createComponentSlot(FormLabel)({ | ||
componentName: "ReadOnlySwitchField", |
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.
componentName: "ReadOnlySwitchField", | |
componentName: "ReadOnlyFieldLabel", |
color: ${theme.palette.grey[900]}; | ||
font-family: ${theme.typography.fontFamily}; | ||
font-size: 16px; | ||
font-style: normal; | ||
font-weight: 600; | ||
line-height: 20px; | ||
letter-spacing: 0px; |
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.
Isn't this a standard typography?
id="readOnlyField.fileField.size" | ||
defaultMessage="{size}" | ||
values={{ | ||
size: `${(file.size / 1024).toFixed(2)} MB`, |
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.
We have a PrettyBytes component for this.
<Wrapper className={className}> | ||
{label && <ReadOnlyFieldLabel label={label} />} | ||
{value && ( | ||
<Box> | ||
<TextContent>{value}</TextContent> | ||
<Lock /> | ||
</Box> | ||
)} | ||
</Wrapper> |
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.
Most for the readonly fields are pretty similar. What do you think about creating a base component that supports label and adornments that is then used by the components?
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.
you could also use FieldContainer
); | ||
|
||
const TextContent = createComponentSlot("span")({ | ||
componentName: "ReadOnlyMultiSelectField", |
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.
Please check all component names if they're correct.
"@comet/admin": minor | ||
--- | ||
|
||
Add Read Only Field Component |
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.
Add Read Only Field Component | |
Add `ReadOnlyField` component |
value?: Date | string | undefined; | ||
className?: string; | ||
}> = ({ label, value, className }) => { | ||
const date = value instanceof Date ? value : value ? new Date(value) : undefined; |
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.
For dates without time we use string (e.g., 2025-03-06
). This field should work with strings as well.
<Box> | ||
<InnerBox> | ||
<Calendar /> | ||
{date && date.toLocaleDateString()} |
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.
We should use FormattedDate like we do for dataGridDateColumn: https://github.com/vivid-planet/comet/blob/main/packages/admin/admin/src/dataGrid/gridColumnTypes.tsx#L7.
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.
Wouldn't it be possible to add multiple support to ReadOnlySelectField? We don't have a MultiSelectField component.
There are some more Field components, e.g. CheckboxListField. Do we plan to add readonly versions for these as well? https://github.com/vivid-planet/comet/tree/main/packages/admin/admin/src/form/fields |
Description
Until now, we have supported read-only fields using a
readOnly
prop on the field component. This has some drawbacks:Comparison
readOnly
Prop on Field- Styling is already correct
- Form state contains values that are not editable
ReadOnlyField
- No need for complex fields (e.g., Selects that load data asynchronously)
- No filtering needed in
onSubmit
- Slightly more effort to use
Plan
Therefore, we want to additionally offer a
ReadOnlyField
component. This will support multiple types and render them correctly (e.g., String / Number / Boolean / Date / File / Block — see table below). The values will be passed directly to the field, not managed via the form state.For block content, the read-only mode should simply show the
PreviewContent
.We still plan to continue supporting the
readOnly
prop on regular fields to make it easy to build dynamic forms.Acceptance criteria
Screenshots/screencasts
Boolean Readonly Field
Date Readonly Field
File Readonly Field
Files Readonly Field
Select Readonly Field
MultiSelect Readonly Field
Number Readonly Field
Rich Text Readonly Field
Text Readonly Field
Text Area Readonly Field
Null and Undefined Value Readonly Fields
Open TODOs/questions
Further information