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

Admin Generator (Future): Implement group type for combination column #2627

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jamesricky
Copy link
Contributor

@jamesricky jamesricky commented Oct 12, 2024

Description

The group type in combination columns is used to group multiple values, separated by a .
This makes it easier than creating a custom formattedMessage and allows empty values instead of requiring an emptyText to prevent results like €13.00 • Shirt • • •.

Another change worth mentioning: the default emptyText value of "-" was changed to an empty string "", as with a single dash, it's unclear what this value was supposed to be, and when nested in groups, the empty text should not be shown in most cases, to prevent results like €13.00 • Shirt • - • - •. Instead, a custom emptyText should be defined in the config when relevant.

Screenshot of Overview column in ProductGrid

Before After
Overview column previously Overview column now

Related tasks and documents

https://vivid-planet.atlassian.net/browse/SVK-368

Open TODOs/questions

  • Merge parent PR Admin Generator (Future): Implement formattedMessage type for combination column #2553
  • Find a way to generate a unique formatted-message ID for when a child-field has no field e.g., for fields of type "group" or "formattedMessage" (the current value is __TODO__)
  • Make sure the variableName groupValues is always unique, e.g. when primaryText and secondaryText both use the "group" type or when there are nested "group" types.
  • Figure out what the default value of emptyValue should be. Currently it is "-". For the "group" type it would make sense to be an empty string, so empty values are not joined into the value-separated string. Maybe it should be consistent with non-group fields, so always an empty string, not just in the "group" type.

@jamesricky jamesricky self-assigned this Oct 12, 2024
@jamesricky jamesricky force-pushed the admin-gen-combination-column-formatted-message-values branch from 778150b to 4755a33 Compare October 29, 2024 07:03
Base automatically changed from admin-gen-combination-column-formatted-message-values to main October 29, 2024 07:33
@jamesricky jamesricky force-pushed the admin-gen-combination-column-group-type branch from a9cb4b5 to e666e91 Compare December 13, 2024 11:09
@jamesricky jamesricky force-pushed the admin-gen-combination-column-group-type branch from e666e91 to b13e54d Compare December 13, 2024 11:28
@jamesricky jamesricky changed the title Admin Generator (Future): Implement "group" type for combination column Admin Generator (Future): Implement group type for combination column Dec 13, 2024
@jamesricky jamesricky marked this pull request as ready for review December 13, 2024 14:19
@auto-assign auto-assign bot requested a review from johnnyomair December 13, 2024 14:19
@jamesricky jamesricky requested review from nsams and johnnyomair and removed request for johnnyomair December 13, 2024 14:19
filterable: false,
sortable: false,
renderCell: ({ row }) => {
const secondaryTextNestedItem3GroupValues: string[] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the "3" come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From here: nestedItem${(index + 1).toString()}

nestingKeys: [
...nestingContext.nestingKeys,
typeof field !== "string" && "field" in field ? field.field : `nestedItem${(index + 1).toString()}`,
],

This is required to make sure message-IDs and variable-names from items in a group are named unique, as unlike the formattedMessage type, the values of a group type do not have a unique key (fields is an array, instead of an object):

{
    type: "formattedMessage",
    message: "Product: {title} is of category {category}",
    valueFields: {
        title: "title",
        category: { type: "text", field: "category.title" },
    },
};
{
    type: "group",
    fields: [
        { type: "text", field: "title" },
        { type: "number", field: "price", currency: "EUR" },
    ],
}

Therefore removing index as a unique value would cause the following issues:

Using multiple type: "static" values would create two equal message-IDs with different text:

primaryText: {
    type: "formattedMessage",
    message: "Static values: ({staticValues})",
    valueFields: {
        staticValues: {
            type: "group",
            fields: [
                { type: "static", text: "Example static text" },
                { type: "static", text: "Another static text" },
            ],
        },
    },
}

// results in:
const primaryTextStaticValuesGroupValues: string[] = [
    intl.formatMessage({ id: "product.groupInFormattedMessage.primaryText.staticValues.nestedItem", defaultMessage: `Example static text` }),
    intl.formatMessage({ id: "product.groupInFormattedMessage.primaryText.staticValues.nestedItem", defaultMessage: `Another static text` }),
];

And would cause this to create two variables with the same name:

primaryText: {
    type: "group",
    fields: [
        {
            type: "group",
            fields: [
                { type: "text", field: "title" },
                { type: "text", field: "type" },
            ],
        },
        {
            type: "group",
            fields: [
                { type: "text", field: "category.title" },
                { type: "text", field: "description" },
            ],
        },
    ],
},

// results in 
const primaryTextNestedItemGroupValues: string[] = [row.title ?? "", row.type ?? ""];
const primaryTextNestedItemGroupValues: string[] = [row.category?.title ?? "", row.description ?? ""];
const primaryTextGroupValues: string[] = [
    primaryTextNestedItemGroupValues.filter(Boolean).join(" • "),
    primaryTextNestedItemGroupValues.filter(Boolean).join(" • "),
];

I know these examples are very niece/unrealistic for a practical use-case, however this makes sure future potential types that require unique values (message-IDs, variables) will work when used inside type: "group".

An alternative, would be to require a key for each item in fields (use an object instead of an array).
Imo an array is easier to configure though.

Comment on lines +331 to +337
const secondaryTextGroupValues: string[] = [
typeof row.price === "undefined" || row.price === null
? ""
: intl.formatNumber(row.price, { minimumFractionDigits: 2, maximumFractionDigits: 2, style: "currency", currency: "EUR" }),
row.category?.title ?? "",
secondaryTextNestedItem3GroupValues.filter(Boolean).join(" • "),
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is complicated to read (and probably) maintain. Should we add a component to @comet/admin to help with this? I'm thinking of something like this:

<CombinedText 
  separator="°"
  segments={[row.price,  row.category?.title]}
/>

(ignore the naming for now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how easy this would be, as theoretically groups can be nested infinitely inside other types (group, formattedMessage). Rendering a react component might cause some issues.

I could try this out in a follow-up task though, if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

2 participants