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.

Sign up for GitHub

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

Jump to bottom

RFC: Demo Admin Products Form: add nested object as fields #1393

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from
+120 −77
Draft

RFC: Demo Admin Products Form: add nested object as fields #1393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions demo/admin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@
"prettier": "^2.0.0",
"style-loader": "^3.0.0",
"ts-node": "^10.0.0",
"type-fest": "^4.6.0",
"typescript": "^4.0.0",
"webpack": "^5.0.0",
"webpack-cli": "^4.0.0",
Expand Down
5 changes: 5 additions & 0 deletions demo/admin/src/products/ProductForm.gql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ export const productFormFragment = gql`
id
title
}
packageDimensions {
width
height
depth
}
}
`;

Expand Down
54 changes: 49 additions & 5 deletions demo/admin/src/products/ProductForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
FinalFormSaveSplitButton,
FinalFormSelect,
FinalFormSubmitEvent,
FormSection,
Loading,
MainContent,
Toolbar,
Expand All @@ -29,6 +30,7 @@ import { filter } from "graphql-anywhere";
import isEqual from "lodash.isequal";
import React from "react";
import { FormattedMessage } from "react-intl";
import { MergeDeep } from "type-fest";

import {
createProductMutation,
Expand Down Expand Up @@ -62,10 +64,18 @@ const rootBlocks = {
image: DamImageBlock,
};

type FormValues = Omit<GQLProductFormManualFragment, "price" | "image"> & {
price: string;
image: BlockState<typeof rootBlocks.image>;
};
type FormValues = MergeDeep<
GQLProductFormManualFragment,
{
price: string;
image: BlockState<typeof rootBlocks.image>;
packageDimensions: {
width: string;
height: string;
depth: string;
};
}
>;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we "need"? Shouldn't this be also achievable using the (now removed) Omit statement? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

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)


function ProductForm({ id }: FormProps): React.ReactElement {
const stackApi = useStackApi();
Expand All @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

width: String(data.product.packageDimensions.width),
height: String(data.product.packageDimensions.height),
depth: String(data.product.packageDimensions.depth),
},
Comment on lines +97 to +102
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of stringifying/parsing we could also parse on field level, like we did for SpaceBlock. I don't know what I prefer though 🤷🏼‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

I will test that out. Sounds good, even the type modification could be avoided with that.

Copy link
Member Author

@nsams nsams Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

See also Mr. final-form himself response here: https://stackoverflow.com/a/55121050

}
: {
image: rootBlocks.image.defaultValues(),
Expand Down Expand Up @@ -113,7 +129,12 @@ function ProductForm({ id }: FormProps): React.ReactElement {
variants: [],
articleNumbers: [],
discounts: [],
packageDimensions: { width: 0, height: 0, depth: 0 },
packageDimensions: {
...formValues.packageDimensions,
width: parseFloat(formValues.packageDimensions.width),
height: parseFloat(formValues.packageDimensions.height),
depth: parseFloat(formValues.packageDimensions.depth),
},
statistics: { views: 0 },
};
if (mode === "edit") {
Expand Down Expand Up @@ -262,6 +283,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" />}>
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe that should be a Fieldset, but that is not the important thing here

Copy link
Collaborator

Choose a reason for hiding this comment

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

We're adding the new FieldSet in #1082

<Field
fullWidth
name="packageDimensions.width"
component={FinalFormInput}
inputProps={{ type: "number" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't type="number" work as well?

label={<FormattedMessage id="product.width" defaultMessage="Width" />}
/>
<Field
fullWidth
name="packageDimensions.height"
component={FinalFormInput}
inputProps={{ type: "number" }}
label={<FormattedMessage id="product.height" defaultMessage="Height" />}
/>
<Field
fullWidth
name="packageDimensions.depth"
component={FinalFormInput}
inputProps={{ type: "number" }}
label={<FormattedMessage id="product.depth" defaultMessage="Depth" />}
/>
</FormSection>
</MainContent>
</EditPageLayout>
)}
Expand Down
4 changes: 2 additions & 2 deletions demo/api/schema.gql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ type Product implements DocumentInterface {
discounts: [ProductDiscounts!]!
articleNumbers: [String!]!
dimensions: ProductDimensions
packageDimensions: ProductPackageDimensions
packageDimensions: ProductPackageDimensions!
statistics: ProductStatistics
createdAt: DateTime!
category: ProductCategory
Expand Down Expand Up @@ -1116,7 +1116,7 @@ input ProductInput {
discounts: [ProductDiscountsInput!]! = []
articleNumbers: [String!]! = []
dimensions: ProductDimensionsInput
packageDimensions: ProductPackageDimensionsInput
packageDimensions: ProductPackageDimensionsInput!
statistics: ProductStatisticsInput
variants: [ProductVariantInput!]! = []
category: ID = null
Expand Down
6 changes: 3 additions & 3 deletions demo/api/src/products/entities/product.entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ export class Product extends BaseEntity<Product, "id"> implements DocumentInterf
@Field(() => ProductDimensions, { nullable: true })
dimensions?: ProductDimensions = undefined;

@Embedded(() => ProductPackageDimensions, { nullable: true })
@Field(() => ProductPackageDimensions, { nullable: true })
packageDimensions?: ProductPackageDimensions = undefined;
@Embedded(() => ProductPackageDimensions)
@Field(() => ProductPackageDimensions)
packageDimensions: ProductPackageDimensions;
Copy link
Member Author

Choose a reason for hiding this comment

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

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


@OneToOne(() => ProductStatistics, { inversedBy: "product", owner: true, ref: true, nullable: true })
@Field(() => ProductStatistics, { nullable: true })
Expand Down
6 changes: 3 additions & 3 deletions demo/api/src/products/generated/dto/product.input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ export class ProductInput {
@Field(() => ProductDimensions, { nullable: true })
dimensions?: ProductDimensions;

@IsNullable()
@IsNotEmpty()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? Shouldn't ValidateNested be enough?

@ValidateNested()
@Type(() => ProductPackageDimensions)
@Field(() => ProductPackageDimensions, { nullable: true })
packageDimensions?: ProductPackageDimensions;
@Field(() => ProductPackageDimensions)
packageDimensions: ProductPackageDimensions;

@IsNullable()
@Field(() => ProductStatisticsInput, { nullable: true })
Expand Down
Loading
Loading