Skip to content

FCN-667: replace raw PF internal CSS with FormSection component - #402

Open
d3mdis wants to merge 2 commits into
RedHatInsights:mainfrom
d3mdis:FCN-667
Open

d3mdis wants to merge 2 commits into
RedHatInsights:mainfrom
d3mdis:FCN-667

Conversation

@d3mdis

@d3mdis d3mdis commented Sep 14, 2026

Copy link
Copy Markdown

Description

Refactor Section to use PatternFly 6’s Form and FormSection components, including the native title prop and section styling. Removed custom CSS that duplicated PatternFly’s form section title layout while preserving existing section content.

Jira issue #
FCN-667

Backport of #

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/enhancement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • UI/UX improvement
  • Code refactoring (no functional changes)
  • Performance improvement
  • Tests
  • Configuration change
  • Infrastructure/build change
  • Other (please specify):

Testing

Manual Testing

Test Steps:

  1. Open both local and dev storybook instances
  2. Simply compare the two and make sure they both use the same margins/paddings(they look identical)

Test Environment:

  • Tested locally
  • Tested in Storybook

Automated Testing

E2E Run:

  • Playwright Tests: E2E tests completed successfully (npm run test:e2e)

Unit Tests:

  • Unit tests added/updated
  • All unit tests pass (npm run test)
  • Ran new/updated files in Stryker

Integration Tests:

  • Integration tests added/updated
  • All integration tests pass

Screenshots/Recordings

Before

image

After

image

Checklist

Code Quality

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have removed any console logs and debugging code
  • My changes generate no new warnings or errors
  • I have checked for and fixed any linting errors (npm run lint)
  • Code formatting is correct (npm run prettier:fix)

Documentation

  • I have updated the documentation accordingly
  • I have updated the README if necessary
  • I have added/updated Storybook stories for new/modified components
  • I have updated TypeScript types/interfaces

Accessibility

  • My changes follow accessibility best practices
  • Interactive elements are keyboard accessible
  • Proper ARIA labels and roles are used where needed
  • Color contrast meets WCAG standards

Dependencies

  • Any dependent changes have been merged and published
  • I have updated package dependencies if needed

Breaking Changes

Does this PR introduce breaking changes?

  • Yes
  • No

Additional Notes

Reviewer Guidelines

Focus Areas

Questions for Reviewers


@d3mdis
d3mdis requested a review from a team as a code owner September 14, 2026 15:51
@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d0386814-a182-4a68-b8d6-dbb7d04d42a6


Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

FCN-667: Adopt PatternFly FormSection for wizard sections

✨ Enhancement 🕐 Less than 10 minutes

Grey Divider

AI Description

• Replaces manually styled sections with PatternFly FormSection and its native title API.
• Preserves conditional Form wrapping, help content, actions, descriptions, and child rendering.
Diagram

graph TD
  Section["Section component"] --> Mode{"Form mode?"} -->|Yes| Form["PatternFly Form"] --> FormSection["FormSection"] --> Content["Section content"]
  Mode -->|No| FormSection
  FormSection --> Title["Native title"]
Loading
High-Level Assessment

Using PatternFly's supported FormSection component is the preferred approach because it removes duplicated internal styling while preserving existing behavior. Retaining custom form-group markup or CSS would remain coupled to PatternFly implementation details and require unnecessary maintenance.

Files changed (2) +10 / -17

Refactor (2) +10 / -17
Section.cssRemove duplicated PatternFly section-title styling +0/-7

Remove duplicated PatternFly section-title styling

• Removes the custom title class and token-based styling now supplied by FormSection. The grouped body-copy layout remains unchanged.

packages/nxtcm-rosa-hcp-wizard/src/components/Section.css

Section.tsxRender wizard sections with PatternFly FormSection +10/-10

Render wizard sections with PatternFly FormSection

• Replaces manual form-group and form-class wrappers with FormSection in both form and non-form modes. Passes the existing composite header through the native title prop while preserving IDs, descriptions, help, actions, and children.

packages/nxtcm-rosa-hcp-wizard/src/components/Section.tsx

@qodo-code-review

qodo-code-review Bot commented Sep 14, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Section omits its UI guidance note 📘 Rule violation ⚙ Maintainability
Description
Section.tsx begins with the PatternFly import instead of a file-level comment referencing
docs/agent-rules/ui-component.md. Because this modified UI component has no pointer to its
applicable guidance, later changes can overlook the component-specific conventions documented there.
Code

packages/nxtcm-rosa-hcp-wizard/src/components/Section.tsx[1]

+import { Content, Form, FormSection, Split, SplitItem, Stack } from '@patternfly/react-core';
Evidence
PR Compliance ID 2644187 requires every modified UI component implementation to reference
docs/agent-rules/ui-component.md in its first non-empty file-level comment. The modified file
starts directly with an import and therefore has no such comment.

Rule 2644187: Component code must reference the matching task-type guidance document in a file-level comment
packages/nxtcm-rosa-hcp-wizard/src/components/Section.tsx[1-4]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The modified `Section.tsx` component lacks the required top-level reference to `docs/agent-rules/ui-component.md`.

## Fix Focus Areas
- packages/nxtcm-rosa-hcp-wizard/src/components/Section.tsx[1-1]

## Recommended Fix
Insert a file-level comment referencing `docs/agent-rules/ui-component.md` before the first import, making it the first non-empty line.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 36 rules
Review mode: ⚖️ Balanced: This is a localized UI refactor, but it changes rendered component structure and PatternFly FormSection semantics in both form and non-form paths, so a careful standard review is warranted.

Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@@ -1,4 +1,4 @@
import { Content, Form, Split, SplitItem, Stack } from '@patternfly/react-core';
import { Content, Form, FormSection, Split, SplitItem, Stack } from '@patternfly/react-core';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Section omits its ui guidance note 📘 Rule violation ⚙ Maintainability

Section.tsx begins with the PatternFly import instead of a file-level comment referencing
docs/agent-rules/ui-component.md. Because this modified UI component has no pointer to its
applicable guidance, later changes can overlook the component-specific conventions documented there.
Agent Prompt
## Issue description
The modified `Section.tsx` component lacks the required top-level reference to `docs/agent-rules/ui-component.md`.

## Fix Focus Areas
- packages/nxtcm-rosa-hcp-wizard/src/components/Section.tsx[1-1]

## Recommended Fix
Insert a file-level comment referencing `docs/agent-rules/ui-component.md` before the first import, making it the first non-empty line.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@vishsanghishetty

Copy link
Copy Markdown
Contributor

Review step - before / after (title regression)

image image

Comment thread packages/nxtcm-rosa-hcp-wizard/src/components/Section.tsx Outdated
@kelvah
kelvah self-requested a review September 15, 2026 10:58
if (isForm) {
return (
<Form onSubmit={(e) => e.preventDefault()}>
<FormSection id={id} title={formSectionTitle}>

@kelvah kelvah Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Now that I look at the code I think the ticket is misleading. (Sorry, I created it, I should've done a better analysis).

FormSection should be used to break down long forms into smaller sections. This happens only in the "Roles and policies" step of the wizard. For the rest of the wizard Section is used only once per step, just to add a Title and a wrapping Form. These are two separate use cases that should not be mixed.

If you agree I think we should use Section (maybe we could use better naming given html also as a section tag) just to wrap each step of the wizard. The title can be just a Title in a Content block outside the form in this case.

We can then use plain FormSections in the Roles step and avoid having two separate forms inside of it, which is weird.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good visually now. the Cluster details / Roles / Machine pool titles are back to heading-xs, and Review still lines up with the YAML button.

I agree with @kelvah on the structure though. FormSection is for splitting a long form, which is only the Roles step and everywhere else Section is just a step title plus a wrapping Form. Mixing those in one component is going to keep biting us.

I’d rather Section stay the step wrapper (title outside the form is fine), and Roles be one Form with two plain FormSections. right now that step still mounts two forms.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, that makes sense, I agree. I'll try to rework it that way.

Comment on lines 60 to +62
<Stack>
<Split hasGutter>
<SplitItem isFilled>
<div className="rosa-hcp-section__title pf-v6-u-w-100">
{label}
{idProp && (
<LabelHelp id={idProp} labelHelp={labelHelp} labelHelpTitle={labelHelpTitle} />
)}
</div>
</SplitItem>
{labelActions ? <SplitItem>{labelActions}</SplitItem> : null}
</Split>
{description && (
<Content component="small" className="pf-v6-u-pt-sm">
{description}
</Content>
)}
{sectionHeader}
{sectionDescription}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This still works but the canonical pattern you see on PF docs is to use StackItem inside Stack

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