CONSOLE-4447: Migrate topology, helm, metal modals to PatternFly v6#16080
CONSOLE-4447: Migrate topology, helm, metal modals to PatternFly v6#16080rhamilto wants to merge 4 commits intoopenshift:mainfrom
Conversation
Update Cancel button variants from "secondary" to "link" in modern PatternFly modals to follow PatternFly standards. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… wrapper - Migrate useCopyCodeModal, FavoriteButton, and TourStepComponent from deprecated Modal wrapper to modern PatternFly Modal components - Remove deprecated Modal wrapper (packages/console-shared/src/components/modal/) - Migrate CatalogDetailsModal and operator-hub-items from deprecated PatternFly Modal to modern Modal components - Preserve ocs-modal CSS class for catalog modal positioning - Fix FavoriteButton form submission bug by adding preventDefault - Fix Guided Tour accessibility warning by closing Help dropdown and blurring focus before starting tour Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Migrate DeleteModal from deprecated factory/modal components to modern PatternFly v6 Modal components - Create reusable ModalFooterWithAlerts component for alert display - Update configure-count-modal and configure-machine-autoscaler-modal to use modern Modal components and ModalFooterWithAlerts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Migrate EditApplicationModal, MoveConnectionModal, and HelmReadmeModal from deprecated modal components to modern PatternFly v6 Modal components. Changes: - Replace ModalWrapper, ModalTitle, ModalBody, ModalSubmitFooter with PatternFly v6 Modal, ModalHeader, ModalBody components - Use ModalFooterWithAlerts for consistent error display - Replace HTML form elements with PatternFly Form component - Update modal providers to use modern Modal with proper state management - Maintain backward compatibility with existing modal launcher hooks Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
component/helm
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
changed the title
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request executes a comprehensive modal UI refactoring across the OpenShift Console codebase. The custom Modal wrapper component in console-shared is removed entirely, with all modal implementations migrated to PatternFly's native Modal components using explicit ModalHeader, ModalBody, and ModalFooter composition. A new ModalFooterWithAlerts component is introduced to standardize footer rendering with integrated error and info message handling. Cancel button variants are changed from "secondary" to "link" across approximately 13 modal implementations. Additionally, focus management is improved in the masthead toolbar, and the InfoMessage utility is exported for broader usage. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx (1)
1-26:⚠️ Potential issue | 🟡 MinorAdd data-test-id to Modal component for test consistency.
The migration pattern is solid, but the Modal component in
HelmReadmeModalProvideris missing adata-test-idattribute. The button launcher hasdata-test-id="helm-readme-modal"in HelmInstallUpgradeForm.tsx, but the Modal itself (line 36-38) should carry a corresponding identifier for e2e test selectors to interact with and verify the modal's presence. Adddata-test-id="helm-readme-modal"to the Modal component to maintain testing infrastructure consistency as noted in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx` around lines 1 - 26, The Modal rendered for the README needs a stable test selector: in the HelmReadmeModal component (function HelmReadmeModal) add the attribute data-test-id="helm-readme-modal" to the Modal element (the Modal component imported from `@patternfly/react-core`) so the e2e tests can find the dialog that corresponds to the launcher button.frontend/packages/console-app/src/components/tour/TourStepComponent.tsx (1)
112-120:⚠️ Potential issue | 🟡 MinorReplace the redundant inner
ModalBodywith a standard container.The code nests a
ModalBodyinside anotherModalBody(lines 112 and 119). PatternFly v6's Modal pattern expectsModalHeader,ModalBody, andModalFooteras direct children ofModal, not layered. The innerModalBodywrappingstepContentis unnecessary—use a plain<div>instead to maintain proper modal semantics and avoid redundant body styling.Suggested refactor
- <ModalBody>{stepContent}</ModalBody> + <div>{stepContent}</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/tour/TourStepComponent.tsx` around lines 112 - 120, The component TourStepComponent nests a ModalBody inside another ModalBody which breaks PatternFly modal semantics; replace the inner ModalBody that wraps stepContent with a plain container (e.g., a div) so ModalHeader, the content container, and ModalFooter are direct children of the outer ModalBody (or Modal) as intended; update the JSX where ModalBody contains ModalHeader, ModalBody (inner), and ModalFooter to instead render ModalHeader, a simple div for stepContent, and ModalFooter, keeping references to ModalBody, ModalHeader, ModalFooter, and stepContent to locate the change.
🧹 Nitpick comments (5)
frontend/packages/metal3-plugin/src/components/modals/StartNodeMaintenanceModal.tsx (1)
62-112: Optional: Consider wrapping form content in a<Form>element for keyboard accessibility.The modal contains form inputs (TextInput for reason) but submission only works via button click. Wrapping content in PatternFly's
<Form onSubmit={submit}>and usingtype="submit"on the primary button would enable Enter-key submission—a common user expectation. Not part of the current change scope, but aligns with the broader modal migration pattern mentioned in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/metal3-plugin/src/components/modals/StartNodeMaintenanceModal.tsx` around lines 62 - 112, Wrap the modal's input area in a PatternFly <Form onSubmit={submit}> so Enter submits the form: move the Stack (or PfModalBody children that include the TextInput bound to reason via setReason) inside a <Form> and set the primary Button to type="submit" instead of onClick; if the existing submit function signature is not (event) => void, either update submit to accept the form event and call event.preventDefault() before existing logic or wrap the call in an inline onSubmit handler that prevents default and then calls submit to avoid double submits.frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx (1)
28-40: State management pattern is correct; minor redundancy note.The provider's internal
isOpenstate combined withcloseOverlay()follows the pattern established across other modals in this migration. The sequence of setting local state tofalsebefore callingcloseOverlay()ensures clean unmounting.Minor observation: The
isOpenprop on line 36 is redundant since the entire<Modal>is already conditionally rendered based on theisOpenstate. PatternFly Modal acceptsisOpenfor controlled rendering, but here you're doing conditional rendering yourself. Both approaches work—just pick one for consistency across the codebase.♻️ Optional: Remove redundant isOpen prop
return isOpen ? ( - <Modal variant={ModalVariant.large} isOpen onClose={handleClose}> + <Modal variant={ModalVariant.large} onClose={handleClose}> <HelmReadmeModal {...props} /> </Modal> ) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx` around lines 28 - 40, The Modal is being conditionally rendered using local state isOpen and also given an isOpen prop redundantly; update HelmReadmeModalProvider to be consistent by removing the isOpen prop from the Modal (keep the conditional rendering and handleClose that calls props.closeOverlay), referencing the HelmReadmeModalProvider component, its isOpen state and handleClose function, and the Modal element so reviewers can locate and remove the unnecessary isOpen attribute.frontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsx (1)
26-30: Consider exporting the props type from the module's public API.
ModalFooterWithAlertsPropsis defined but not exported. If consumers need to type-annotate variables or extend this component, exporting the props type would be helpful:-type ModalFooterWithAlertsProps = { +export type ModalFooterWithAlertsProps = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsx` around lines 26 - 30, Export the props type so consumers can reference it: change the local type declaration ModalFooterWithAlertsProps to an exported type (export type ModalFooterWithAlertsProps = { ... }) and, if this module is re-exported from a barrel/public API, add ModalFooterWithAlertsProps to that module's exports so it becomes part of the package's public API.frontend/public/components/modals/configure-count-modal.tsx (1)
84-103: Use form-native submit wiring for consistent keyboard behavior.Line 84 defines a form, but submission is still click-driven from Line 101. Wiring submit to the form keeps Enter-key behavior and prevents divergence across modal implementations.
♻️ Suggested change
- <Form id="configure-count-form"> + <Form id="configure-count-form" onSubmit={submit}> @@ - <Button + <Button variant="primary" isLoading={inProgress} - onClick={submit} + type="submit" form="configure-count-form" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/configure-count-modal.tsx` around lines 84 - 103, The form is declared with id "configure-count-form" but the submit logic is attached to the Button's onClick (submit) which breaks native Enter-key behavior; update the Form component to handle onSubmit={submit} (and prevent default inside submit if necessary), change the Button to be a plain submit button (type="submit") and remove the Button's onClick and form props so the modal uses native form submission; reference the Form id "configure-count-form", the submit handler named submit, and the Button component to locate and update the code.frontend/packages/console-shared/src/hooks/useCopyCodeModal.tsx (1)
7-14: Clean migration to PF v6 Modal composition.The structure with explicit
ModalHeaderandModalBodychildren is the correct PatternFly v6 pattern. TheisOpenalways beingtrueis appropriate here since the modal provider handles conditional rendering.One small nit: the
key={snippet}onCopyToClipboard(line 11) appears unnecessary. Since the modal is unmounted when closed and remounted when launched again, React will recreate the component regardless. Thekeywould only matter if the parent remained mounted whilesnippetchanged, which isn't the case with this launcher pattern. Harmless, but could be removed for clarity.♻️ Optional: Remove unnecessary key prop
<ModalBody> - <CopyToClipboard key={snippet} value={snippet} /> + <CopyToClipboard value={snippet} /> </ModalBody>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/hooks/useCopyCodeModal.tsx` around lines 7 - 14, The CopyToClipboard component inside the CopyCodeModal is being given an unnecessary React key (key={snippet}); since the modal is unmounted/remounted by the modal provider, the key is redundant—remove the key prop from the CopyToClipboard usage in CopyCodeModal to simplify the markup (locate the CopyCodeModal functional component and remove the key={snippet} attribute on the CopyToClipboard element).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx`:
- Around line 35-39: The Modal instance rendering HelmReadmeModal lacks
aria-labelledby so the custom ModalHeader title isn't exposed to assistive tech;
update the Modal invocation (the Modal with variant={ModalVariant.large} isOpen
onClose={handleClose}) to include aria-labelledby="helm-readme-modal-title" and
change the HelmReadmeModal component signature to accept a titleId prop (e.g.,
titleId?: string) and pass that into ModalHeader via its labelId prop
(ModalHeader labelId={titleId}); alternatively, move the title into the Modal's
title prop so PatternFly wires aria-labelledby automatically.
In `@frontend/packages/topology/src/components/modals/EditApplicationModal.tsx`:
- Line 127: The spread props are placed after provider-managed handlers so
caller props can override the provider's handleClose (which calls
closeOverlay()); for EditApplicationModal and MoveConnectionModal move ...props
before the explicit cancel={handleClose} and close={handleClose} entries so the
provider's handleClose cannot be overridden by incoming props, ensuring
closeOverlay() always runs.
In `@frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx`:
- Line 200: The spread props are currently passed after the handlers which
allows callers to override critical close behavior; update the JSX so the
component receives {...props} first and then explicit handlers last (e.g. change
MoveConnectionModal cancel/close order to <MoveConnectionModal {...props}
cancel={handleClose} close={handleClose} />) so the component's cancel/close
handlers (used to call props.closeOverlay()) cannot be overridden by incoming
MoveConnectionModalProps.
In `@frontend/public/components/modals/configure-machine-autoscaler-modal.tsx`:
- Around line 139-141: The Cancel button's onClick uses closeOverlay ||
cancelProp but should match the submit fallback logic by including close as the
final fallback; update the Button in ConfigureMachineAutoscalerModal so its
onClick resolves to closeOverlay || cancelProp || close (using the same close
symbol used by the submit handler) to ensure consistent defensive handling.
---
Outside diff comments:
In `@frontend/packages/console-app/src/components/tour/TourStepComponent.tsx`:
- Around line 112-120: The component TourStepComponent nests a ModalBody inside
another ModalBody which breaks PatternFly modal semantics; replace the inner
ModalBody that wraps stepContent with a plain container (e.g., a div) so
ModalHeader, the content container, and ModalFooter are direct children of the
outer ModalBody (or Modal) as intended; update the JSX where ModalBody contains
ModalHeader, ModalBody (inner), and ModalFooter to instead render ModalHeader, a
simple div for stepContent, and ModalFooter, keeping references to ModalBody,
ModalHeader, ModalFooter, and stepContent to locate the change.
In
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx`:
- Around line 1-26: The Modal rendered for the README needs a stable test
selector: in the HelmReadmeModal component (function HelmReadmeModal) add the
attribute data-test-id="helm-readme-modal" to the Modal element (the Modal
component imported from `@patternfly/react-core`) so the e2e tests can find the
dialog that corresponds to the launcher button.
---
Nitpick comments:
In
`@frontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsx`:
- Around line 26-30: Export the props type so consumers can reference it: change
the local type declaration ModalFooterWithAlertsProps to an exported type
(export type ModalFooterWithAlertsProps = { ... }) and, if this module is
re-exported from a barrel/public API, add ModalFooterWithAlertsProps to that
module's exports so it becomes part of the package's public API.
In `@frontend/packages/console-shared/src/hooks/useCopyCodeModal.tsx`:
- Around line 7-14: The CopyToClipboard component inside the CopyCodeModal is
being given an unnecessary React key (key={snippet}); since the modal is
unmounted/remounted by the modal provider, the key is redundant—remove the key
prop from the CopyToClipboard usage in CopyCodeModal to simplify the markup
(locate the CopyCodeModal functional component and remove the key={snippet}
attribute on the CopyToClipboard element).
In
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx`:
- Around line 28-40: The Modal is being conditionally rendered using local state
isOpen and also given an isOpen prop redundantly; update HelmReadmeModalProvider
to be consistent by removing the isOpen prop from the Modal (keep the
conditional rendering and handleClose that calls props.closeOverlay),
referencing the HelmReadmeModalProvider component, its isOpen state and
handleClose function, and the Modal element so reviewers can locate and remove
the unnecessary isOpen attribute.
In
`@frontend/packages/metal3-plugin/src/components/modals/StartNodeMaintenanceModal.tsx`:
- Around line 62-112: Wrap the modal's input area in a PatternFly <Form
onSubmit={submit}> so Enter submits the form: move the Stack (or PfModalBody
children that include the TextInput bound to reason via setReason) inside a
<Form> and set the primary Button to type="submit" instead of onClick; if the
existing submit function signature is not (event) => void, either update submit
to accept the form event and call event.preventDefault() before existing logic
or wrap the call in an inline onSubmit handler that prevents default and then
calls submit to avoid double submits.
In `@frontend/public/components/modals/configure-count-modal.tsx`:
- Around line 84-103: The form is declared with id "configure-count-form" but
the submit logic is attached to the Button's onClick (submit) which breaks
native Enter-key behavior; update the Form component to handle onSubmit={submit}
(and prevent default inside submit if necessary), change the Button to be a
plain submit button (type="submit") and remove the Button's onClick and form
props so the modal uses native form submission; reference the Form id
"configure-count-form", the submit handler named submit, and the Button
component to locate and update the code.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (29)
frontend/packages/console-app/src/components/favorite/FavoriteButton.tsxfrontend/packages/console-app/src/components/modals/add-group-users-modal.tsxfrontend/packages/console-app/src/components/tour/TourStepComponent.tsxfrontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.scssfrontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsxfrontend/packages/console-shared/src/components/index.tsfrontend/packages/console-shared/src/components/modal/Modal.tsxfrontend/packages/console-shared/src/components/modal/index.tsfrontend/packages/console-shared/src/components/modals/CreateNamespaceModal.tsxfrontend/packages/console-shared/src/components/modals/CreateProjectModal.tsxfrontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsxfrontend/packages/console-shared/src/hooks/useCopyCodeModal.tsxfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsxfrontend/packages/knative-plugin/src/components/test-function/TestFunctionModal.tsxfrontend/packages/metal3-plugin/src/components/modals/PowerOffHostModal.tsxfrontend/packages/metal3-plugin/src/components/modals/RestartHostModal.tsxfrontend/packages/metal3-plugin/src/components/modals/StartNodeMaintenanceModal.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsxfrontend/packages/operator-lifecycle-manager/src/components/registry-poll-interval-details.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/packages/topology/src/components/modals/MoveConnectionModal.tsxfrontend/public/components/masthead/masthead-toolbar.tsxfrontend/public/components/modals/configure-count-modal.tsxfrontend/public/components/modals/configure-machine-autoscaler-modal.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/modals/delete-namespace-modal.tsxfrontend/public/components/secrets/create-secret/SecretFormWrapper.tsxfrontend/public/components/utils/button-bar.tsxfrontend/public/locales/en/public.json
💤 Files with no reviewable changes (3)
- frontend/packages/console-shared/src/components/modal/index.ts
- frontend/packages/console-shared/src/components/index.ts
- frontend/packages/console-shared/src/components/modal/Modal.tsx
| return isOpen ? ( | ||
| <Modal variant={ModalVariant.large} isOpen onClose={handleClose}> | ||
| <HelmReadmeModal {...props} /> | ||
| </Modal> | ||
| ) : null; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PatternFly v6 Modal aria-labelledby ModalHeader accessibility
💡 Result:
In PatternFly v6, the modal’s accessible name comes from either:
titleon<Modal>(PatternFly will auto-setaria-labelledbyfor you), oraria-labelledbyon<Modal>that points at the ID of the element that visually labels the modal—this is required when you use a custom header / theheaderapproach instead oftitle. [1]
With the v6 React subcomponents, that typically means:
- Set an ID on the header title via
<ModalHeader labelId="my-modal-title" ... /> - Point the modal at it via
<Modal aria-labelledby="my-modal-title" ... />(you can include multiple IDs if needed). [1][2]
Example:
<Modal
isOpen
onClose={onClose}
aria-labelledby="my-modal-title"
>
<ModalHeader labelId="my-modal-title">My modal title</ModalHeader>
<ModalBody>...</ModalBody>
</Modal>Notes:
- If there is no visible title, use
aria-labelon<Modal>instead. [1] - If you provide a description with
<ModalHeader descriptorId="..." />, the modal’saria-describedbyshould match thatdescriptorId(or the relevant header ID), per the docs. [2]
Sources: PatternFly Modal accessibility + props documentation. [1][2]
References:
[1] PatternFly Modal – Accessibility (aria-labelledby requirements; title auto-wiring) https://www.patternfly.org/components/modal/accessibility/
[2] PatternFly Modal – React props (Modal aria-labelledby; ModalHeader labelId / descriptorId) https://www.patternfly.org/components/modal/
🌐 Web query:
PatternFly v6 Modal accessibility best practices ARIA attributes
💡 Result:
PatternFly v6 Modal: accessibility + ARIA best practices
Required / expected ARIA on the modal container
- Use
role="dialog"on the modal container (.pf-v6-c-modal-box). [1] - Use
aria-modal="true"on the modal container. (PatternFly marks this as required for its HTML modal.) [1]
Give the modal an accessible name (required)
Use one of:
aria-labelledby="ID_OF_VISIBLE_TITLE"(recommended when you have a visible title). [1]aria-label="Text title"if there is no visible text title. [1]
PatternFly React props that map to this
title(automatically setsaria-labelledby) oraria-labelledby(if you provide a custom header), oraria-label(if no visible title). [1]
Provide an accessible description (recommended when there’s static “primary message” text)
- Add
aria-describedby="ID_OF_PRIMARY_CONTENT"pointing to the element that describes the modal’s purpose/message. [1], [2]
PatternFly React prop
aria-describedby. [1]
Close control (required)
- Ensure there is at least one clickable way to close the modal (close icon, Cancel button, etc.). [1]
- If the close control is an icon-only button, give it an accessible name (e.g.,
aria-label="Close"). [1]
Make the background inert + trap focus (required behavior for a modal)
- While open, users must not be able to tab to or interact with page content outside the modal (focus trap). [1], [3]
- PatternFly’s guidance for screen readers is to set
aria-hidden="true"on the parent wrapper of the page content (with the modal rendered outside that wrapper as a sibling), so screen readers can’t navigate the background. [1], [3] - If you set
aria-modal="true", ensure the dialog actually behaves modally (no background interaction), per ARIA practices guidance. [2]
PatternFly React prop
- Avoid
disableFocusTrapunless you implement your own focus management correctly. [1]
If the modal body scrolls (overflow)
- Give the scrollable body an accessible name and role, and make it keyboard-focusable:
- Body:
aria-label="…",role="region"(typical), andtabindex="0"when the body is scrollable. [1]
- Body:
PatternFly React props
bodyAriaLabel(and optionallybodyAriaRole). [1]
🏁 Script executed:
# Locate and examine HelmReadmeModal component
find . -name "HelmReadmeModal.tsx" -type fRepository: openshift/console
Length of output: 149
🏁 Script executed:
# Find the actual HelmReadmeModal component and examine its implementation
fd -t f "HelmReadmeModal" --exec cat -n {} \;Repository: openshift/console
Length of output: 1935
Add aria-labelledby to Modal; PatternFly v6 does NOT auto-associate custom ModalHeader with the modal's accessible name.
When using a custom <ModalHeader> composition (rather than Modal's title prop), you must explicitly set aria-labelledby on <Modal> pointing to the labelId on <ModalHeader>. Currently, the modal has no accessible name, so screen readers won't announce the README title on open.
<Modal
variant={ModalVariant.large}
isOpen
aria-labelledby="helm-readme-modal-title"
onClose={handleClose}
>
<HelmReadmeModal {...props} titleId="helm-readme-modal-title" />
</Modal>Update HelmReadmeModal to accept and pass the title ID:
const HelmReadmeModal: FunctionComponent<Props & { titleId?: string }> = ({ readme, theme, titleId }) => {
const { t } = useTranslation();
return (
<>
<ModalHeader title={t('helm-plugin~README')} labelId={titleId} />
<ModalBody>
<SyncMarkdownView content={readme} theme={theme} />
</ModalBody>
</>
);
};Alternatively, simplify by moving the title to Modal's title prop (PatternFly auto-wires aria-labelledby for you).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsx`
around lines 35 - 39, The Modal instance rendering HelmReadmeModal lacks
aria-labelledby so the custom ModalHeader title isn't exposed to assistive tech;
update the Modal invocation (the Modal with variant={ModalVariant.large} isOpen
onClose={handleClose}) to include aria-labelledby="helm-readme-modal-title" and
change the HelmReadmeModal component signature to accept a titleId prop (e.g.,
titleId?: string) and pass that into ModalHeader via its labelId prop
(ModalHeader labelId={titleId}); alternatively, move the title into the Modal's
title prop so PatternFly wires aria-labelledby automatically.
|
|
||
| return isOpen ? ( | ||
| <Modal variant={ModalVariant.small} isOpen onClose={handleClose}> | ||
| <EditApplicationModal cancel={handleClose} close={handleClose} {...props} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/packages/topology/src/components/modals/EditApplicationModal.tsxRepository: openshift/console
Length of output: 5507
🏁 Script executed:
find . -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "MoveConnectionModal" | head -5Repository: openshift/console
Length of output: 191
🏁 Script executed:
cat -n frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx | head -150Repository: openshift/console
Length of output: 5838
🏁 Script executed:
cat -n frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx | tail -100Repository: openshift/console
Length of output: 3682
🏁 Script executed:
# Verify React prop precedence behavior by checking TypeScript compilation
# Look for any tests that might demonstrate the expected behavior
find . -path ./node_modules -prune -o -type f \( -name "*.test.tsx" -o -name "*.spec.tsx" \) -print | xargs grep -l "EditApplicationModal\|MoveConnectionModal" | head -3Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check what props might be passed to these modals by looking at how they're launched
rg -A 5 "useEditApplicationModalLauncher\|useEditApplicationModal" frontend/packages/topology/src --type tsx | head -40Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Check what props might be passed to these modals by looking at how they're launched
rg "useEditApplicationModalLauncher|useEditApplicationModal" frontend/packages/topology/src -A 5 | head -50Repository: openshift/console
Length of output: 1839
🏁 Script executed:
# Check the OverlayComponent type to understand the full interface
rg "type OverlayComponent" --type-list | head -1
rg "type OverlayComponent" -A 10 | head -20Repository: openshift/console
Length of output: 1731
🏁 Script executed:
# Verify the actual types and see what could be passed through props
cat -n frontend/packages/topology/src/actions/modify-application.ts | head -60Repository: openshift/console
Length of output: 1381
🏁 Script executed:
# Check if there are other modals with similar pattern in topology
rg "Provider.*props.*=>" frontend/packages/topology/src/components/modals/ -A 10 | grep -E "(Provider|<.*cancel=|close=|\.\.\.))"Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Look for similar modal provider patterns to see if any were fixed differently
rg "Provider.*OverlayComponent" frontend/packages/topology/src/components/modals/ -A 15 | grep -B 5 -A 10 "cancel=\|close="Repository: openshift/console
Length of output: 2901
🏁 Script executed:
# Double-check that the current code order is indeed problematic by looking at EditApplicationForm
# which uses the cancel prop
cat -n frontend/packages/topology/src/components/modals/EditApplicationModal.tsx | grep -A 2 -B 2 "cancel"Repository: openshift/console
Length of output: 751
Reorder props to prevent spread from overriding provider-managed cleanup handlers.
Line 127 places ...props after cancel/close, allowing caller-provided props to override the provider's handleClose function. This breaks the provider pattern since handleClose contains the essential closeOverlay() cleanup call.
Suggested fix
- <EditApplicationModal cancel={handleClose} close={handleClose} {...props} />
+ <EditApplicationModal {...props} cancel={handleClose} close={handleClose} />Note: MoveConnectionModal has the same pattern on line 200 and should be fixed identically.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <EditApplicationModal cancel={handleClose} close={handleClose} {...props} /> | |
| <EditApplicationModal {...props} cancel={handleClose} close={handleClose} /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/topology/src/components/modals/EditApplicationModal.tsx` at
line 127, The spread props are placed after provider-managed handlers so caller
props can override the provider's handleClose (which calls closeOverlay()); for
EditApplicationModal and MoveConnectionModal move ...props before the explicit
cancel={handleClose} and close={handleClose} entries so the provider's
handleClose cannot be overridden by incoming props, ensuring closeOverlay()
always runs.
|
|
||
| return isOpen ? ( | ||
| <Modal variant={ModalVariant.small} isOpen onClose={handleClose}> | ||
| <MoveConnectionModal cancel={handleClose} close={handleClose} {...props} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd frontend/packages/topology && find . -name "MoveConnectionModal.tsx" -type fRepository: openshift/console
Length of output: 108
🏁 Script executed:
cd frontend/packages/topology && head -220 src/components/modals/MoveConnectionModal.tsx | tail -40Repository: openshift/console
Length of output: 1127
🏁 Script executed:
cd frontend/packages/topology && cat -n src/components/modals/MoveConnectionModal.tsx | sed -n '180,220p'Repository: openshift/console
Length of output: 1200
🏁 Script executed:
cd frontend/packages/topology && cat -n src/components/modals/MoveConnectionModal.tsx | head -100Repository: openshift/console
Length of output: 3844
🏁 Script executed:
cd frontend/packages/topology && rg "interface MoveConnectionModalProps|type MoveConnectionModalProps" -A 15Repository: openshift/console
Length of output: 1108
🏁 Script executed:
cd frontend/packages/topology && rg "const MoveConnectionModal" -A 5Repository: openshift/console
Length of output: 1055
Ensure spread props cannot override critical close handlers.
Line 200 passes cancel/close handlers before ...props, allowing caller-provided props to override handleClose and bypass props.closeOverlay(). Since MoveConnectionModalProps declares both as optional fields, this is exploitable.
🐛 Suggested fix
- <MoveConnectionModal cancel={handleClose} close={handleClose} {...props} />
+ <MoveConnectionModal {...props} cancel={handleClose} close={handleClose} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <MoveConnectionModal cancel={handleClose} close={handleClose} {...props} /> | |
| <MoveConnectionModal {...props} cancel={handleClose} close={handleClose} /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx` at
line 200, The spread props are currently passed after the handlers which allows
callers to override critical close behavior; update the JSX so the component
receives {...props} first and then explicit handlers last (e.g. change
MoveConnectionModal cancel/close order to <MoveConnectionModal {...props}
cancel={handleClose} close={handleClose} />) so the component's cancel/close
handlers (used to call props.closeOverlay()) cannot be overridden by incoming
MoveConnectionModalProps.
| <Button variant="link" onClick={closeOverlay || cancelProp} type="button"> | ||
| {t('public~Cancel')} | ||
| </Button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 150 frontend/public/components/modals/configure-machine-autoscaler-modal.tsx | cat -nRepository: openshift/console
Length of output: 5828
🏁 Script executed:
rg -A 5 "type ConfigureMachineAutoscalerModalProps" frontend/public/components/modals/configure-machine-autoscaler-modal.tsxRepository: openshift/console
Length of output: 185
🏁 Script executed:
rg "interface OverlayComponent|type OverlayComponent" -A 10Repository: openshift/console
Length of output: 1324
🏁 Script executed:
rg "closeOverlay" frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support/OverlayProvider.tsx -B 5 -A 5Repository: openshift/console
Length of output: 1282
🏁 Script executed:
rg "ConfigureMachineAutoscalerModal" -B 2 -A 2 --type tsx --type ts | head -100Repository: openshift/console
Length of output: 88
🏁 Script executed:
rg "ConfigureMachineAutoscalerModal" -B 2 -A 2 | head -100Repository: openshift/console
Length of output: 2494
🏁 Script executed:
cat -n frontend/packages/console-app/src/actions/hooks/useMachineSetActions.ts | grep -A 10 "ConfigureMachineAutoscalerModal"Repository: openshift/console
Length of output: 1269
Align Cancel fallback with Submit to ensure consistency.
The submit handler explicitly falls back to close (lines 81–82), but the Cancel button only uses closeOverlay || cancelProp. While the current usage pattern relies on closeOverlay being guaranteed by OverlayProvider, the inconsistency limits defensive handling and could confuse future maintainers. Align the Cancel handler with submit logic:
- <Button variant="link" onClick={closeOverlay || cancelProp} type="button">
+ <Button variant="link" onClick={closeOverlay || cancelProp || close} type="button">
{t('public~Cancel')}
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button variant="link" onClick={closeOverlay || cancelProp} type="button"> | |
| {t('public~Cancel')} | |
| </Button> | |
| <Button variant="link" onClick={closeOverlay || cancelProp || close} type="button"> | |
| {t('public~Cancel')} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/configure-machine-autoscaler-modal.tsx`
around lines 139 - 141, The Cancel button's onClick uses closeOverlay ||
cancelProp but should match the submit fallback logic by including close as the
final fallback; update the Button in ConfigureMachineAutoscalerModal so its
onClick resolves to closeOverlay || cancelProp || close (using the same close
symbol used by the submit handler) to ensure consistent defensive handling.
|
@rhamilto: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Migrates topology, helm, metal modals to modern PatternFly v6 Modal components.
Note: This PR includes changes from #16015, so it should merge first.
Files Migrated So Far
packages/topology/src/components/modals/EditApplicationModal.tsxpackages/topology/src/components/modals/MoveConnectionModal.tsxpackages/helm-plugin/src/components/forms/install-upgrade/HelmReadmeModal.tsxTODO - Files Not Yet Updated
Note: these files have pending changes in other PRs that complicate them being updated.
packages/topology/src/components/export-app/ExportApplicationModal.tsxpackages/metal3-plugin/src/components/modals/PowerOffHostModal.tsxpackages/metal3-plugin/src/components/modals/RestartHostModal.tsxpackages/metal3-plugin/src/components/modals/StartNodeMaintenanceModal.tsxChanges
Component Updates
ModalWrapper,ModalTitle,ModalBody,ModalSubmitFooterwith PatternFly v6 componentsModal,ModalHeader,ModalBody,Button,ModalVariantfrom@patternfly/react-coreModalFooterWithAlertsfor consistent error display across modalsForm Improvements
<form>elements with PatternFlyFormcomponentpf-v6-c-formclass)type="submit"andformattribute on buttonsModal Provider Pattern
Modalcomponent with proper state managementTesting
Manual Testing Locations
EditApplicationModal:
MoveConnectionModal:
HelmReadmeModal:
Test Coverage
Related PRs
🤖 Generated with Claude Code