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

fix(278): Group parameters in the configuration panel #975

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

tplevko
Copy link
Contributor

@tplevko tplevko commented Mar 26, 2024

Fix for #278

with this change, the fields with label "advanced" are grouped under expandable element.

The behaviour for e.g. the ActiveMq component:

vokoscreenNG-2024-03-26_14-31-12.mp4

@tplevko tplevko force-pushed the issue_278 branch 4 times, most recently from f440156 to f51c9f9 Compare March 27, 2024 09:01
@tplevko tplevko marked this pull request as ready for review March 27, 2024 09:46
@tplevko tplevko requested a review from lordrip March 27, 2024 09:46
</Card>
);
},
);
Copy link
Member

@lordrip lordrip Mar 30, 2024

Choose a reason for hiding this comment

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

After checking it, I think it's a good approach, but there's still an open question for the future, not exactly related to this PR which is "how to identify components in consumer or producer mode", so we can filter such properties.

I tinkered a bit with your idea and I have a proposal, please let me know what you think:

import { Card, CardBody, CardHeader, CardTitle, ExpandableSection } from '@patternfly/react-core';
import { HTMLFieldProps, connectField, filterDOMProps } from 'uniforms';
import { getValue } from '../../../utils';
import { CustomAutoField } from '../CustomAutoField';
import './CustomNestField.scss';

export type CustomNestFieldProps = HTMLFieldProps<
  object,
  HTMLDivElement,
  { properties?: Record<string, unknown>; helperText?: string; itemProps?: object }
>;

export const CustomNestField = connectField(
  ({
    children,
    error,
    errorMessage,
    fields,
    itemProps,
    label,
    name,
    showInlineError,
    disabled,
    ...props
  }: CustomNestFieldProps) => {
    const propertiesArray = Object.entries(props.properties ?? {}).reduce(
      (acc, [name, definition]) => {
        let labels: string = getValue(definition, 'labels', '');
        if (labels === '' || labels === 'common' || labels === 'producer' || labels === 'consumer') {
          acc.common.push(name);
          return acc;
        }

        if (labels.includes('advanced')) {
          acc.groups['advanced'] ??= [];
          acc.groups['advanced'].push(name);
          return acc;
        }

        acc.groups[labels] ??= [];
        acc.groups[labels].push(name);
        return acc;
      },
      { common: [], groups: {} } as { common: string[]; groups: Record<string, string[]> },
    );

    return (
      <Card data-testid={'nest-field'} {...filterDOMProps(props)} isExpanded>
        <CardHeader>
          <CardTitle>{label}</CardTitle>
        </CardHeader>
        <CardBody className="custom-nestfield-spacing">
          {propertiesArray.common.map((field) => (
            <CustomAutoField key={field} name={field} />
          ))}
        </CardBody>

        {Object.entries(propertiesArray.groups).map(([groupName, groupFields]) => (
          <ExpandableSection toggleText={`${groupName} properties`}>
            <CardBody className="custom-nestfield-spacing">
              {groupFields.map((field) => (
                <CustomAutoField key={field} name={field} />
              ))}
            </CardBody>
          </ExpandableSection>
        ))}
      </Card>
    );
  },
);

The difference here is that we iterate over the labels to create the groups dynamically, this way, for amqp we show other groups:
image

It's not a final idea, since we would need to decorate the expandable titles with sentence case, and maybe there are other rough edges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like there are quite many groups, but this is I suppose only the case for handful of connectors with large number of config properties, so even though for AMQP this looks like too many config dropdowns, this probably would be OK for most of the connectors.

The one thing that comes to my mind is how would we deduplicate the properties in the config, as in this solution one property could appear in more than one dropdown if I'm not mistaking.

Copy link
Contributor

@lhein lhein Apr 2, 2024

Choose a reason for hiding this comment

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

Nope, each parameter can be only in one group which is determined by the catalog attribute "group".
Also keep in mind to use the attribute "label" for the group names.

Example:

 "group": "logging", "label": "consumer,logging",

Copy link
Member

Choose a reason for hiding this comment

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

Got it, I think it's possible to adjust it to match that, I'm just curious to know @tplevko opinion on the approach 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm...actually I am not sure label is a good idea. that sounds more like tags

Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

In an ideal world, we should be able to accomplish the same without creating our own CustomNestField, but I think we can move forward with this approach and reevaluate at a later stage.

The reason why I mention this is that the more we bring common elements from uniforms-patternfly, the more we duplicate code and the library becomes a bit more complex to maintain.

Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

Awesome, great job @tplevko, this is how the amqp component looks like now:

image

As we talked about it, there are a few little topics that we might want to revisit in a separate PR:

  • Reduce the space between the group title and the first field
  • Capitalize the first word of the group title
  • Maybe putting Advanced always at the end

@lordrip lordrip merged commit 96dba69 into KaotoIO:main Apr 3, 2024
9 checks passed
@lhein
Copy link
Contributor

lhein commented Apr 3, 2024

Can we capitalize the group names?

@lordrip
Copy link
Member

lordrip commented Apr 3, 2024

Can we capitalize the group names?

Yup, there are a couple of options, one with CSS and another one with a function call. A PR is coming soon.

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.

3 participants