fix layout issue with field input

[ingridsmoreira]

Description

Issue: #2263
Wrap the component field in the ThemeAwareFormGroupWrapper
added new optional value to the ThemeAwareFormGroupWrapper because the logic to make it dirty was not correct

Before:

After:

PF Mode:

How Has This Been Tested?

That was manually tested, no tests are added since is just a visible change in UX

Merge criteria:

• All the commits have been signed-off (To pass the DCO check)

• The commits have meaningful messages
• Automated tests are provided as part of the PR for major new functionalities; testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work.
• Code changes follow the kubeflow contribution guidelines.
• For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

If you have UI changes

• The developer has added tests or explained why testing cannot be added.
• Included any necessary screenshots or gifs if it was a UI change.
• Verify that UI/UX changes conform the UX guidelines for Kubeflow.

[manaswinidas]

Thanks for the fix! The core approach of wrapping the organization field in ThemeAwareFormGroupWrapper is correct.

However, the isInvalid prop feels like a workaround for a design gap in ThemeAwareFormGroupWrapper. The real issue is that the component conflates "has helper text" with "has error" via !!helperTextNode. That works for callers like CreateModal where helperTextNode is conditionally false when there's no error, but breaks here because the organization field bundles always-visible description text with conditional error text into a fragment — and a fragment <>...</> is always truthy.

Instead of adding isInvalid, I'd suggest adding a description prop to ThemeAwareFormGroupWrapper for always-visible help text, keeping helperTextNode strictly for error text. This way !!helperTextNode continues to correctly infer error state, no extra boolean needed.

ThemeAwareFormGroupWrapper.tsx — add description, drop isInvalid, keep hasError = !!helperTextNode:

type ThemeAwareFormGroupWrapperProps = {
  children: React.ReactNode;
  label: string;
  fieldId: string;
  isRequired?: boolean;
  description?: React.ReactNode;    // Always-visible help text
  helperTextNode?: React.ReactNode; // Error-only helper text
  className?: string;
  labelHelp?: React.ReactElement;
  'data-testid'?: string;
};

Then render description after the input (before helperTextNode) in both theme branches:

// MUI theme
<FormGroup ...>
  <FormFieldset component={children} field={label} />
</FormGroup>
{description}
{helperTextNode}

// PF theme
<FormGroup ...>
  {children}
  {description}
  {helperTextNode}
</FormGroup>

CredentialsSection.tsx — split the text, match the pattern used in CreateModal:

const organizationDescription = (
  <FormHelperText>
    <HelperText>
      <HelperTextItem>{HELP_TEXT.ORGANIZATION}</HelperTextItem>
    </HelperText>
  </FormHelperText>
);

const organizationHelperTextNode = isOrganizationTouched && !isOrganizationValid && (
  <FormHelperText>
    <HelperText>
      <HelperTextItem variant="error" data-testid="organization-error">
        {VALIDATION_MESSAGES.ORGANIZATION_REQUIRED}
      </HelperTextItem>
    </HelperText>
  </FormHelperText>
);

<ThemeAwareFormGroupWrapper
  label={FORM_LABELS.ORGANIZATION}
  fieldId="organization"
  isRequired
  description={organizationDescription}
  helperTextNode={organizationHelperTextNode}
>
  {organizationInput}
</ThemeAwareFormGroupWrapper>

This keeps the API clean, avoids duplicating state (isInvalid just mirrors helperTextNode truthiness), and matches the existing pattern used across CreateModal.

Also a minor nit: missing semicolon after the organizationHelperTextNode closing ) on line 83.

[manaswinidas]

@ingridsmoreira also fix the linting issues while you are addressing the review comments - alternatively you can use npm run test on the frontend folder and run the UI - Frontend test suite locally to detect and fix any linting, type check issues, unit and Cypress test failures

[ingridsmoreira]

@manaswinidas Thank you for the insights, I added the changes you requested and fixed the lint.

[manaswinidas]

Tested locally - is there a way in which the helpertext appears in the same place as before this change. Right now it is below the input field. Earlier it was above. This also affects the PF theme. The screenshot is taken in Patternfly theme btw

Reading order is wrong. Right now, both description and error render below the input. The description is static guidance the user should see before interacting with the field, so it belongs above the input. The error ("Organization is required") is feedback after interaction, so it should appear immediately below the input - not after the description text.

can we try something like this?

PF theme:
<FormGroup ...>
{descriptionTextNode}
{children}
{helperTextNode}

MUI theme:
<>
<FormGroup ...>
{descriptionTextNode}


{helperTextNode}
</>

This keeps the order as label -> description -> input -> error, and ensures hasError correctly detects the error state to apply pf-m-error.

[ingridsmoreira]

Tested locally - is there a way in which the helpertext appears in the same place as before this change. Right now it is below the input field. Earlier it was above. This also affects the PF theme. The screenshot is taken in Patternfly theme btw

Reading order is wrong. Right now, both description and error render below the input. The description is static guidance the user should see before interacting with the field, so it belongs above the input. The error ("Organization is required") is feedback after interaction, so it should appear immediately below the input - not after the description text.

can we try something like this?

PF theme: <FormGroup ...> {descriptionTextNode} {children} {helperTextNode}

MUI theme: <> <FormGroup ...> {descriptionTextNode} {helperTextNode} </>

This keeps the order as label -> description -> input -> error, and ensures hasError correctly detects the error state to apply pf-m-error.

Implementing the suggested change in PF mode becomes this:

That looks ok.

But for the suggested change in non-PF mode it becomes this:

Almost the same issue that opened the bug.

The code of this is this one:

<>
        <FormGroup
          className={`${className || ''} ${hasError ? 'pf-m-error' : ''}`.trim()} // Apply className and error state class
          label={label}
          isRequired={isRequired}
          fieldId={fieldId}
          labelHelp={labelHelp}
          data-testid={dataTestId}
        >
          {descriptionTextNode}
          <FormFieldset component={children} field={label} />
        </FormGroup>
        {helperTextNode}
      </>

If we move the description outside of the FormGroup it becomes like this

The code for these changes are this:

<>
        {descriptionTextNode}
        <FormGroup
          className={`${className || ''} ${hasError ? 'pf-m-error' : ''}`.trim()} // Apply className and error state class
          label={label}
          isRequired={isRequired}
          fieldId={fieldId}
          labelHelp={labelHelp}
          data-testid={dataTestId}
        >
          <FormFieldset component={children} field={label} />
        </FormGroup>
        {helperTextNode}
      </>

I will push with this last changes, that looks better(at least to me lol) but let me know your thoughts @manaswinidas and thank you again for the review and suggestions

[jonburdo]

/ok-to-test

[Philip-Carneiro]

On my view, it's good to go, it solve the issue I opened

[google-oss-prow]

@Philip-Carneiro: changing LGTM is restricted to collaborators

Details

In response to this:

On my view, it's good to go, it solve the issue I opnned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[manaswinidas]

LGTM - just needs a rebase and then I'll approve

[manaswinidas]

The changes look good in both PF theme and MUI theme

[ingridsmoreira]

LGTM - just needs a rebase and then I'll approve

Rebased

[manaswinidas]

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: manaswinidas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• clients/ui/OWNERS [manaswinidas]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment