RFC: Demo Admin Products Form: add nested object as fields #1393
Conversation
| packageDimensions?: ProductPackageDimensions = undefined; | ||
| @Embedded(() => ProductPackageDimensions) | ||
| @Field(() => ProductPackageDimensions) | ||
| packageDimensions: ProductPackageDimensions; |
There was a problem hiding this comment.
I make the field non-nullable as:
- it is broken currently, as the database fields (of the embedded) are non-nullable
- it simplifies the form; I'd discuss the simpler one first
| @@ -262,6 +277,29 @@ function ProductForm({ id }: FormProps): React.ReactElement { | |||
| <Field name="image" isEqual={isEqual}> | |||
| {createFinalFormBlock(rootBlocks.image)} | |||
| </Field> | |||
| <FormSection title={<FormattedMessage id="product.packageDimensions" defaultMessage="Package Dimensions" />}> | |||
There was a problem hiding this comment.
maybe that should be a Fieldset, but that is not the important thing here
There was a problem hiding this comment.
We're adding the new FieldSet in #1082
nsams
force-pushed
the
demo-productsnested-obj
branch
from
187b275
to
4e64ec3
Compare
| depth: string; | ||
| }; | ||
| } | ||
| >; |
There was a problem hiding this comment.
That is very special, uses MergeDeep from a new "type-fest" dependency. Problem is that { foo: string } & { foo: number } results in { foo: string | number }, while Merge results in { foo: number }. And MergeDeep does this also for nested objects.
type-fest is a very popular library
There was a problem hiding this comment.
Do we "need"? Shouldn't this be also achievable using the (now removed) Omit statement? 🤔
There was a problem hiding this comment.
Manual Omit is possible, but very verbose and complicated for nested objects. That's why I was looking for an alternative.
Omit<GQLProductFormManualFragment, "price" | "image" | "packageDimensions"> & {
price: string;
image: BlockState<typeof rootBlocks.image>;
packageDimensions: Omit<GQLProductFormManualFragment["packageDimensions"], "width" | "height" | "depth"> & {
width: string;
height: string;
depth: string;
}
}
(as packageDimensions currently has only those thre fields this could be simplified, but let's assume there are other eg string fields in packageDimensions)
| @@ -69,11 +69,11 @@ export class ProductInput { | |||
| @Field(() => ProductDimensions, { nullable: true }) | |||
| dimensions?: ProductDimensions; | |||
|
|
|||
| @IsNullable() | |||
| @IsNotEmpty() | |||
There was a problem hiding this comment.
Is this necessary? Shouldn't ValidateNested be enough?
| @@ -262,6 +277,29 @@ function ProductForm({ id }: FormProps): React.ReactElement { | |||
| <Field name="image" isEqual={isEqual}> | |||
| {createFinalFormBlock(rootBlocks.image)} | |||
| </Field> | |||
| <FormSection title={<FormattedMessage id="product.packageDimensions" defaultMessage="Package Dimensions" />}> | |||
There was a problem hiding this comment.
We're adding the new FieldSet in #1082
| packageDimensions: { | ||
| ...data.product.packageDimensions, | ||
| width: String(data.product.packageDimensions.width), | ||
| height: String(data.product.packageDimensions.height), | ||
| depth: String(data.product.packageDimensions.depth), | ||
| }, |
There was a problem hiding this comment.
Instead of stringifying/parsing we could also parse on field level, like we did for SpaceBlock. I don't know what I prefer though 🤷🏼♂️
There was a problem hiding this comment.
I will test that out. Sounds good, even the type modification could be avoided with that.
There was a problem hiding this comment.
that would be:
parse={(v: number) => String(v)}
format={(v: string) => parseFloat(v)}
- form state saves value as float (as the input and output api expect it)
- dom input field value is a string (that's how it is)
Major issue: you can't type 1. (and continue with number after the ".")
For non-float numbers (as in the space block) it is no issue, as "." is not supported anyway.
There was a problem hiding this comment.
See also Mr. final-form himself response here: https://stackoverflow.com/a/55121050
| depth: string; | ||
| }; | ||
| } | ||
| >; |
There was a problem hiding this comment.
Do we "need"? Shouldn't this be also achievable using the (now removed) Omit statement? 🤔
| fullWidth | ||
| name="packageDimensions.width" | ||
| component={FinalFormInput} | ||
| inputProps={{ type: "number" }} |
There was a problem hiding this comment.
Doesn't type="number" work as well?
| @@ -84,6 +94,12 @@ function ProductForm({ id }: FormProps): React.ReactElement { | |||
| ...filter<GQLProductFormManualFragment>(productFormFragment, data.product), | |||
| price: String(data.product.price), | |||
| image: rootBlocks.image.input2State(data.product.image), | |||
| packageDimensions: { | |||
| ...data.product.packageDimensions, | |||
There was a problem hiding this comment.
This spread is unnecessary as there's nothing in the dimensions except those three fields. But I believe you want to highlight the required transformations (e.g., a string would be perfectly fine in the spread).
Attention: This will be generated by crud generator, review carefully
When the GraphQL API provides nested objects, how can we handle them best in our forms?