[WIP] CONSOLE-4447: Migrate modals to modern PatternFly Modal components#16015
[WIP] CONSOLE-4447: Migrate modals to modern PatternFly Modal components#16015rhamilto wants to merge 3 commits intoopenshift:mainfrom
Conversation
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. |
|
[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/knative
|
@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 migrates modal implementations across OpenShift Console from a custom Modal wrapper component to PatternFly's native Modal composition (ModalHeader, ModalBody, ModalFooter). Changes include: removing the deprecated custom Modal from console-shared; introducing a new ModalErrorContent component for centralized error rendering; refactoring existing modal structures in multiple packages to use PatternFly's composed modal API; standardizing Cancel button styling from secondary to link variant across modals; updating modal imports and class names; and adjusting modal event handling and closure logic. The custom modal export is removed from the components barrel file. 🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/console-app/src/components/tour/TourStepComponent.tsx (1)
112-120: 🛠️ Refactor suggestion | 🟠 MajorRestructure modal composition to comply with PatternFly v6 patterns—do not nest ModalHeader/ModalBody/ModalFooter inside another ModalBody.
PatternFly v6 Modal expects direct children:
<ModalHeader>(optional),<ModalBody>(required),<ModalFooter>(optional). The current nested structure violates this pattern and introduces semantic issues (duplicaterole="main") and potential a11y/styling regressions.🛠️ Refactor option: use plain divs for nested content layout
- <ModalBody> - <Grid hasGutter> + <Grid hasGutter>Then replace nested PF components with plain wrappers:
- <ModalHeader>{header}</ModalHeader> - <ModalBody>{stepContent}</ModalBody> - <ModalFooter>{footer}</ModalFooter> + <div className="co-tour-step-component__header">{header}</div> + <div className="co-tour-step-component__body">{stepContent}</div> + <div className="co-tour-step-component__footer">{footer}</div>This preserves layout flexibility while maintaining proper Modal composition semantics.
🤖 Fix all issues with AI agents
In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx`:
- Around line 161-190: The form id "confirm-favorite-form" and the Button's form
prop "confirm-favorite" do not match, so clicking Save won't trigger native form
submission/validation; update the Button's form attribute to
"confirm-favorite-form" (the Form with id="confirm-favorite-form") so the Save
Button (the Button that calls handleConfirmStar) is properly bound to the form
and native validation/submission works.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/modals/CreateProjectModal.tsx (1)
121-130: LGTM – Cancel button variant updated.The change aligns with the PR's modal standardization. Button ordering follows PatternFly guidelines.
Minor observation: The Cancel button uses
disabled(line 124) while the primary button usesisDisabled(lines 112-113). Both work, butisDisabledis the PatternFly standard prop. This is a pre-existing inconsistency and outside the scope of this PR, but worth addressing in a future cleanup.
| <Form id="confirm-favorite-form" onSubmit={handleConfirmStar}> | ||
| <FormGroup label={t('Name')} isRequired fieldId="input-name"> | ||
| <TextInput | ||
| id="confirm-favorite-form-name" | ||
| data-test="input-name" | ||
| name="name" | ||
| type="text" | ||
| onChange={(e, v) => handleNameChange(v)} | ||
| value={name || ''} | ||
| autoFocus | ||
| required | ||
| /> | ||
| {error && ( | ||
| <FormHelperText> | ||
| <HelperText> | ||
| <HelperTextItem variant="error">{error}</HelperTextItem> | ||
| </HelperText> | ||
| </FormHelperText> | ||
| )} | ||
| </FormGroup> | ||
| </Form> | ||
| </ModalBody> | ||
| <ModalFooter> | ||
| <Button | ||
| key="confirm" | ||
| variant="primary" | ||
| onClick={handleConfirmStar} | ||
| form="confirm-favorite" | ||
| > | ||
| {t('Save')} |
There was a problem hiding this comment.
Fix the form id mismatch so the Save button is bound to the form.
Right now the form id is confirm-favorite-form, but the button references confirm-favorite, so click won’t trigger native form validation or submission semantics. Update the button’s form attribute to match.
🔧 Suggested fix
- <Button
+ <Button
key="confirm"
variant="primary"
onClick={handleConfirmStar}
- form="confirm-favorite"
+ form="confirm-favorite-form"
>📝 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.
| <Form id="confirm-favorite-form" onSubmit={handleConfirmStar}> | |
| <FormGroup label={t('Name')} isRequired fieldId="input-name"> | |
| <TextInput | |
| id="confirm-favorite-form-name" | |
| data-test="input-name" | |
| name="name" | |
| type="text" | |
| onChange={(e, v) => handleNameChange(v)} | |
| value={name || ''} | |
| autoFocus | |
| required | |
| /> | |
| {error && ( | |
| <FormHelperText> | |
| <HelperText> | |
| <HelperTextItem variant="error">{error}</HelperTextItem> | |
| </HelperText> | |
| </FormHelperText> | |
| )} | |
| </FormGroup> | |
| </Form> | |
| </ModalBody> | |
| <ModalFooter> | |
| <Button | |
| key="confirm" | |
| variant="primary" | |
| onClick={handleConfirmStar} | |
| form="confirm-favorite" | |
| > | |
| {t('Save')} | |
| <Form id="confirm-favorite-form" onSubmit={handleConfirmStar}> | |
| <FormGroup label={t('Name')} isRequired fieldId="input-name"> | |
| <TextInput | |
| id="confirm-favorite-form-name" | |
| data-test="input-name" | |
| name="name" | |
| type="text" | |
| onChange={(e, v) => handleNameChange(v)} | |
| value={name || ''} | |
| autoFocus | |
| required | |
| /> | |
| {error && ( | |
| <FormHelperText> | |
| <HelperText> | |
| <HelperTextItem variant="error">{error}</HelperTextItem> | |
| </HelperText> | |
| </FormHelperText> | |
| )} | |
| </FormGroup> | |
| </Form> | |
| </ModalBody> | |
| <ModalFooter> | |
| <Button | |
| key="confirm" | |
| variant="primary" | |
| onClick={handleConfirmStar} | |
| form="confirm-favorite-form" | |
| > | |
| {t('Save')} |
🤖 Prompt for AI Agents
In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx`
around lines 161 - 190, The form id "confirm-favorite-form" and the Button's
form prop "confirm-favorite" do not match, so clicking Save won't trigger native
form submission/validation; update the Button's form attribute to
"confirm-favorite-form" (the Form with id="confirm-favorite-form") so the Save
Button (the Button that calls handleConfirmStar) is properly bound to the form
and native validation/submission works.
|
@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
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@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
force-pushed
the
CONSOLE-4447
branch
from
648f566 to
bc77836
Compare
|
@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. |
|
/retest |
1 similar comment
|
/retest |
rhamilto
force-pushed
the
CONSOLE-4447
branch
from
bc77836 to
0281ce5
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
rhamilto
force-pushed
the
CONSOLE-4447
branch
from
0281ce5 to
36700bd
Compare
|
/retest |
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>
|
@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
force-pushed
the
CONSOLE-4447
branch
2 times, most recently
from
81869e8 to
6ff28cc
Compare
- Migrate DeleteModal from deprecated factory/modal components to modern PatternFly v6 Modal components - Create reusable ModalErrorContent component for error display - Update configure-count-modal and configure-machine-autoscaler-modal to use modern Modal components and ModalErrorContent - Fix button order in add-group-users-modal (Primary first, Cancel second) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4447
branch
from
6ff28cc to
36f553a
Compare
|
/retest |
|
@rhamilto: The following test 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
This is the first of what will be many PRs.
Summary
This PR migrates several modals from deprecated modal components to modern PatternFly v6 Modal components and updates button variants to follow PatternFly standards.
Changes
Commit 1: Update modal Cancel buttons to use variant="link" (10 files)
Updates Cancel button variants from "secondary" to "link" in modern PatternFly modals to follow PatternFly standards:
public/components/modals/delete-namespace-modal.tsxpublic/components/secrets/create-secret/SecretFormWrapper.tsxpackages/console-app/src/components/modals/add-group-users-modal.tsx- Also fixed button order (Primary first, Cancel second)packages/console-shared/src/components/modals/CreateNamespaceModal.tsxpackages/console-shared/src/components/modals/CreateProjectModal.tsxpackages/metal3-plugin/src/components/modals/PowerOffHostModal.tsxpackages/metal3-plugin/src/components/modals/RestartHostModal.tsxpackages/metal3-plugin/src/components/modals/StartNodeMaintenanceModal.tsxpackages/knative-plugin/src/components/test-function/TestFunctionModal.tsxpackages/operator-lifecycle-manager/src/components/registry-poll-interval-details.tsxBefore
After
Commit 2: Migrate modals to modern PatternFly Modal and remove deprecated Modal wrapper (10 files)
useCopyCodeModal,FavoriteButton, andTourStepComponentfrom deprecated Modal wrapper to modern PatternFly Modalpackages/console-shared/src/components/modal/)CatalogDetailsModalandoperator-hub-itemsfrom deprecated PatternFly Modal to modern Modalocs-modalCSS class for catalog modal positioningModals look the same.
Commit 3: Migrate DeleteModal and related modals to modern PatternFly Modal (5 files)
DeleteModalfrom deprecated factory/modal components to modern PatternFly v6 ModalModalErrorContentcomponent for error display in modal bodiesconfigure-count-modalandconfigure-machine-autoscaler-modalto use modern Modal and ModalErrorContentdelete-modalto modern PatternFly Modal with proper error handlingBefore
After
Note the after error message contents are different because they were taken while running development code. In production,
_HttpError:will not be part of the error message.Key Migration Patterns
ModalErrorContentcomponentvariant="link"(PatternFly standard)Testing
Related Issues
🤖 Generated with Claude Code