CONSOLE-4447: Migrate knative-plugin modals to PatternFly 6#16065
CONSOLE-4447: Migrate knative-plugin modals to PatternFly 6#16065rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
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
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@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 |
2 similar comments
|
/retest |
|
/retest |
|
@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-knative
branch
from
e8d2b1c to
158afe6
Compare
|
/retest |
6 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
rhamilto
force-pushed
the
CONSOLE-4447-knative
branch
from
158afe6 to
d5d0e63
Compare
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
rhamilto
force-pushed
the
CONSOLE-4447-knative
branch
from
d5d0e63 to
1e2584c
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. |
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Tech debt |
openshift-ci
bot
added
docs-approved
|
/retest |
2 similar comments
|
/retest |
|
/retest |
rhamilto
force-pushed
the
CONSOLE-4447-knative
branch
from
1e2584c to
3dc2af9
Compare
rhamilto
force-pushed
the
CONSOLE-4447-knative
branch
from
3dc2af9 to
5f0021c
Compare
📝 WalkthroughWalkthroughThis pull request modernizes modal implementations across the knative-plugin package by migrating from deprecated internal modal components to PatternFly Modal composition patterns. Multiple modal components (pub-sub, sink, traffic-splitting, test-function, delete-revision) transition from ModalTitle/ModalSubmitFooter-based structures to ModalHeader/ModalBody/ModalFooter/ModalFooterWithAlerts compositions. Modal wrapper components are updated to use PatternFly's Modal with explicit isOpen and variant props instead of ModalWrapper. Associated test mocks are updated to reflect the new PatternFly component usage. Supporting changes include test selector updates, property access path corrections for service metadata, and stylesheet adjustments for the test-function modal layout. 🚥 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 |
|
@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
frontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsx (1)
7-16: Don't mock away the footer buttons in this migration test.The risky part of this refactor is the footer wiring (
form="sink-source-form"and the cancel action), butButtonis mocked tonulland the spec submits the form programmatically. This test can still pass even if the migrated footer buttons stop working.🧪 Tighten the spec around the new footer contract
-import { render, fireEvent } from '@testing-library/react'; +import { render, fireEvent, screen } from '@testing-library/react'; @@ jest.mock('@patternfly/react-core', () => ({ ...jest.requireActual('@patternfly/react-core'), ModalHeader: jest.fn(() => null), ModalBody: jest.fn(({ children }) => <div>{children}</div>), - Button: jest.fn(() => null), + Button: jest.fn(({ children, isLoading, isDisabled, variant, ...props }) => ( + <button {...props} disabled={Boolean(isLoading || isDisabled)}> + {children} + </button> + )), Form: jest.fn(({ children, ...props }) => <form {...props}>{children}</form>), })); @@ - it('should render form with id', () => { - const { container } = render(<SinkSourceModal {...formProps} />); - const form = container.querySelector('form'); - expect(form).toBeInTheDocument(); - expect(form).toHaveAttribute('id', 'sink-source-form'); + it('wires the footer submit button to the form', () => { + const { container } = render(<SinkSourceModal {...formProps} />); + expect(container.querySelector('#sink-source-form')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'knative-plugin~Save' })).toHaveAttribute( + 'form', + 'sink-source-form', + ); });Also applies to: 61-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsx` around lines 7 - 16, The test currently mocks away Button and the modal footer (ModalFooterWithAlerts), which removes the real footer buttons and prevents verifying the footer wiring (form="sink-source-form" and cancel action); update the jest.mock for '@patternfly/react-core' to stop overriding Button (remove or return the actual Button via jest.requireActual) and change the mock for '@console/shared/src/components/modals/ModalFooterWithAlerts' to return the real ModalFooterWithAlerts (use jest.requireActual) or at least render its children including any Button elements instead of null so the footer buttons remain in the DOM and the spec can assert the migrated footer contract (look for ModalFooterWithAlerts, Button, and Form in SinkSourceModal.spec.tsx).
🤖 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/knative-plugin/src/components/revisions/DeleteRevisionModal.tsx`:
- Around line 36-41: The confirmation text in DeleteRevisionModal currently uses
three separate t() calls around strong elements which hard-codes English word
order; replace them with a single translation key (using either i18n's t with
interpolation or the Trans component) that accepts placeholders for revision,
service and namespace (pass deleteRevision.metadata.name, serviceName, and
deleteRevision.metadata.namespace) and preserve the <strong> styling by using
Trans's components or t(..., { interpolation }) so translators can reorder words
and punctuation correctly.
In
`@frontend/packages/knative-plugin/src/components/revisions/DeleteRevisionModalController.tsx`:
- Around line 87-94: The Modal's title id is inconsistent between the
early-return ModalHeader ("unable-delete-revision-modal-title") and the
normal-flow title ("delete-revision-modal-title"), preventing the outer Modal
from referencing a stable label; update both ModalHeader usages in
DeleteRevisionModalController.tsx to use a single normalized id (e.g.,
"delete-revision-modal-title") and add aria-labelledby="{that-id}" to the outer
Modal component (the Modal instance rendered in this controller) so the dialog
correctly references the title for accessibility.
In
`@frontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsubModal.spec.tsx`:
- Around line 4-14: Tests currently mock Button to null so they never exercise
the footer->form submit link introduced by the refactor; update the mocks in
SinkPubsubModal.spec.tsx so the Button mock renders an element that
preserves/accepts the form prop (or stop mocking Button entirely) and ensure
ModalFooterWithAlerts mock renders a footer save button with
form="sink-pubsub-form" that can be clicked in the test; then add an assertion
that clicking the footer save button triggers the form submit path (e.g., assert
the form submit handler or the expected post-submit side effect from the Form
component) to cover the new contract between ModalFooterWithAlerts/Button and
the Form.
In
`@frontend/packages/knative-plugin/src/components/sink-source/SinkSourceController.tsx`:
- Around line 20-24: The Modal wrapper in SinkSourceModalProvider is missing an
accessible name; update the Modal element in the SinkSourceModalProvider
component to include aria-labelledby="sink-source-modal-title" so it references
the ModalHeader's labelId (used inside SinkSourceController/ModalHeader),
ensuring the Modal and ModalHeader are connected for accessibility.
In
`@frontend/packages/knative-plugin/src/components/sink-uri/__tests__/SinkUriModal.spec.tsx`:
- Around line 6-24: The current spec only verifies render, so add tests in
SinkUriModal.spec.tsx that exercise the footer-to-form contract: render the
SinkUriModal (or the specific component under test), locate the submit button
rendered via the mocked ModalFooterWithAlerts/Button, assert its form attribute
equals the Form's id (or that the form element has the expected id from the
component), and simulate a click (or fireEvent.submit on the form) to verify the
component's onSubmit/submit handler is invoked; also add a test that ensures a
disabled/missing submit button fails the expected submit behavior. Use the
existing mocked Form, ModalFooterWithAlerts, and Button symbols to find elements
and assert the form-id/form-attribute linkage and actual submit invocation.
In
`@frontend/packages/knative-plugin/src/components/sink-uri/SinkUriController.tsx`:
- Around line 23-25: The Modal wrapper in SinkUriController is missing the
aria-labelledby attribute that should reference the ModalHeader's ID
("sink-uri-modal-title"); update the Modal element in SinkUriController (the
<Modal> that renders <SinkUriController ... />) to include
aria-labelledby="sink-uri-modal-title" so the dialog's accessible name is tied
to the header ID used by SinkUriModal/ModalHeader.
In
`@frontend/packages/knative-plugin/src/components/test-function/TestFunctionController.tsx`:
- Around line 42-44: The Modal instance wrapping TestFunctionController is
missing an accessibility link to the modal header; add
aria-labelledby="test-function-modal-title" to the Modal props so screen readers
can associate the dialog with the TestFunctionController's ModalHeader (which
uses labelId="test-function-modal-title"); keep existing onClose/cancel props
and other props unchanged.
In
`@frontend/packages/knative-plugin/src/components/test-function/TestFunctionModal.tsx`:
- Around line 34-49: The Form onSubmit in TestFunctionModal currently calls
handleSubmit() without the event which can allow a full page reload; update the
Form's onSubmit handler to accept the event parameter, call
event.preventDefault(), and then invoke Formik's handleSubmit with that event
before calling setCurrentView(ModalPanel.Response) (referencing the Form id
"test-function-form", the handleSubmit function and
setCurrentView/ModalPanel.Response symbols) so the default browser submit is
stopped and Formik handles submission correctly.
In
`@frontend/packages/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingModal.spec.tsx`:
- Around line 10-20: The test mock for Button hides the real footer submit
wiring — stop returning null for Button in the jest.mock so the footer submit
button is rendered (or replace the Button mock with one that renders a native
<button> and forwards props like type and onClick); update the test to click the
ModalFooterWithAlerts/Button instance (instead of only calling
fireEvent.submit(form)) to verify the submit button in the footer is actually
wired to the traffic-splitting-form handler (references: mocked Button, Form,
ModalFooterWithAlerts and the traffic-splitting-form submit test that currently
uses fireEvent.submit(form)).
In
`@frontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingController.tsx`:
- Around line 59-65: The Modal wrapper lacks an aria-labelledby that points to
the ModalHeader id; add aria-labelledby="traffic-splitting-modal-title" to the
Modal element that renders TrafficSplittingController so the dialog is properly
labeled (i.e., update the Modal JSX where Modal is opened to include
aria-labelledby="traffic-splitting-modal-title", referencing the existing
labelId on the ModalHeader).
---
Nitpick comments:
In
`@frontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsx`:
- Around line 7-16: The test currently mocks away Button and the modal footer
(ModalFooterWithAlerts), which removes the real footer buttons and prevents
verifying the footer wiring (form="sink-source-form" and cancel action); update
the jest.mock for '@patternfly/react-core' to stop overriding Button (remove or
return the actual Button via jest.requireActual) and change the mock for
'@console/shared/src/components/modals/ModalFooterWithAlerts' to return the real
ModalFooterWithAlerts (use jest.requireActual) or at least render its children
including any Button elements instead of null so the footer buttons remain in
the DOM and the spec can assert the migrated footer contract (look for
ModalFooterWithAlerts, Button, and Form in SinkSourceModal.spec.tsx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bab54361-16fd-4f88-8d2e-9a67062feb30
📒 Files selected for processing (22)
frontend/packages/dev-console/integration-tests/support/pages/modal.tsfrontend/packages/knative-plugin/src/components/pub-sub/PubSubController.tsxfrontend/packages/knative-plugin/src/components/pub-sub/PubSubModal.tsxfrontend/packages/knative-plugin/src/components/revisions/DeleteRevisionModal.tsxfrontend/packages/knative-plugin/src/components/revisions/DeleteRevisionModalController.tsxfrontend/packages/knative-plugin/src/components/sink-pubsub/SinkPubsubController.tsxfrontend/packages/knative-plugin/src/components/sink-pubsub/SinkPubsubModal.tsxfrontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsubModal.spec.tsxfrontend/packages/knative-plugin/src/components/sink-source/SinkSourceController.tsxfrontend/packages/knative-plugin/src/components/sink-source/SinkSourceModal.tsxfrontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsxfrontend/packages/knative-plugin/src/components/sink-uri/SinkUriController.tsxfrontend/packages/knative-plugin/src/components/sink-uri/SinkUriModal.tsxfrontend/packages/knative-plugin/src/components/sink-uri/__tests__/SinkUriModal.spec.tsxfrontend/packages/knative-plugin/src/components/test-function/RequestPane.tsxfrontend/packages/knative-plugin/src/components/test-function/TestFunction.tsxfrontend/packages/knative-plugin/src/components/test-function/TestFunctionController.tsxfrontend/packages/knative-plugin/src/components/test-function/TestFunctionModal.scssfrontend/packages/knative-plugin/src/components/test-function/TestFunctionModal.tsxfrontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingController.tsxfrontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingModal.tsxfrontend/packages/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingModal.spec.tsx
| <p> | ||
| {t('knative-plugin~Are you sure you want to delete ')} | ||
| <strong className="co-break-word">{deleteRevision.metadata.name}</strong>{' '} | ||
| {t('knative-plugin~from ')} <strong className="co-break-word">{serviceName}</strong>{' '} | ||
| {t('knative-plugin~in namespace ')} <strong>{deleteRevision.metadata.namespace}</strong> | ||
| ? |
There was a problem hiding this comment.
Keep the confirmation sentence in one translation key.
The three separate t() calls around the <strong> markers hard-code English word order and punctuation. That won't localize cleanly once translators need to reorder revision, service, and namespace. Please switch this paragraph to a single Trans or interpolated message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/revisions/DeleteRevisionModal.tsx`
around lines 36 - 41, The confirmation text in DeleteRevisionModal currently
uses three separate t() calls around strong elements which hard-codes English
word order; replace them with a single translation key (using either i18n's t
with interpolation or the Trans component) that accepts placeholders for
revision, service and namespace (pass deleteRevision.metadata.name, serviceName,
and deleteRevision.metadata.namespace) and preserve the <strong> styling by
using Trans's components or t(..., { interpolation }) so translators can reorder
words and punctuation correctly.
| <ModalHeader | ||
| title={t('knative-plugin~Unable to delete {{revlabel}}', { | ||
| revlabel: RevisionModel.label, | ||
| })} | ||
| titleIconVariant="danger" | ||
| labelId="unable-delete-revision-modal-title" | ||
| data-test-id="modal-title" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd frontend/packages/knative-plugin && find . -name "DeleteRevisionModalController.tsx" -type fRepository: openshift/console
Length of output: 121
🏁 Script executed:
cat -n frontend/packages/knative-plugin/src/components/revisions/DeleteRevisionModalController.tsx | head -220Repository: openshift/console
Length of output: 8718
🌐 Web query:
PatternFly Modal aria-labelledby accessibility requirement
💡 Result:
In PatternFly, a Modal must have an accessible name. The recommended/default way is to set aria-labelledby on the modal container to the id of the element that visually titles the modal (typically the modal title element, e.g. .pf-v6-c-modal-box__title). This is an accessibility requirement because it gives the dialog its programmatic label. [1], [2]
For PatternFly React specifically:
- Use
aria-labelledbywhen the modal’s label comes from an element in the modal header—PatternFly notes it’s required when you use theheaderprop rather than thetitleprop. [1] - If there is no visible title text to reference, provide an accessible name with
aria-labelinstead (PatternFly notes this is required in that case). [1]
Sources
[1] PatternFly Modal – Accessibility (React) (patternfly.org)
[2] PatternFly Modal – HTML docs (patternfly.org)
Citations:
- 1: https://www.patternfly.org/components/modal/accessibility?utm_source=openai
- 2: https://www.patternfly.org/components/modal/html/?utm_source=openai
🏁 Script executed:
cat -n frontend/packages/knative-plugin/src/components/revisions/DeleteRevisionModal.tsx | head -150Repository: openshift/console
Length of output: 3479
Add aria-labelledby to the outer Modal and normalize the title id across both branches.
PatternFly's Modal component requires an accessible name via aria-labelledby on the outer dialog container. Your code paths render two different title ids—unable-delete-revision-modal-title (early return) and delete-revision-modal-title (normal flow)—making it impossible for the Modal at line 204 to reference a stable id. Normalize to a single title id and add the aria-labelledby attribute to the Modal container.
♿ Fix outline
- labelId="unable-delete-revision-modal-title"
+ labelId="delete-revision-modal-title"
@@
- <Modal isOpen onClose={props.closeOverlay} variant="small">
+ <Modal
+ isOpen
+ onClose={props.closeOverlay}
+ variant="small"
+ aria-labelledby="delete-revision-modal-title"
+ >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/revisions/DeleteRevisionModalController.tsx`
around lines 87 - 94, The Modal's title id is inconsistent between the
early-return ModalHeader ("unable-delete-revision-modal-title") and the
normal-flow title ("delete-revision-modal-title"), preventing the outer Modal
from referencing a stable label; update both ModalHeader usages in
DeleteRevisionModalController.tsx to use a single normalized id (e.g.,
"delete-revision-modal-title") and add aria-labelledby="{that-id}" to the outer
Modal component (the Modal instance rendered in this controller) so the dialog
correctly references the title for accessibility.
| jest.mock('@patternfly/react-core', () => ({ | ||
| ...jest.requireActual('@patternfly/react-core'), | ||
| ModalHeader: jest.fn(() => null), | ||
| ModalBody: jest.fn(({ children }) => <div>{children}</div>), | ||
| Button: jest.fn(() => null), | ||
| Form: jest.fn(({ children, ...props }) => <form {...props}>{children}</form>), | ||
| })); | ||
|
|
||
| jest.mock('@console/shared/src/components/modals/ModalFooterWithAlerts', () => ({ | ||
| ModalFooterWithAlerts: jest.fn(({ children }) => <div>{children}</div>), | ||
| })); |
There was a problem hiding this comment.
These tests no longer cover the migrated submit path.
Because Button is mocked to null and the assertions stop at “renders”, this spec will not catch a broken form="sink-pubsub-form" link between the footer save button and the form. That is the key behavior introduced by this refactor.
Minimal way to exercise the new contract
jest.mock('@patternfly/react-core', () => ({
...jest.requireActual('@patternfly/react-core'),
ModalHeader: jest.fn(() => null),
ModalBody: jest.fn(({ children }) => <div>{children}</div>),
- Button: jest.fn(() => null),
+ Button: jest.fn(({ children, ...props }) => <button {...props}>{children}</button>),
Form: jest.fn(({ children, ...props }) => <form {...props}>{children}</form>),
}));
@@
- it('should render with all required props', () => {
- const { container } = render(<SinkPubsubModal {...formProps} />);
- expect(container).toBeInTheDocument();
+ it('wires the footer save button to the form', () => {
+ const { getByRole } = render(<SinkPubsubModal {...formProps} />);
+ expect(getByRole('button', { name: 'knative-plugin~Save' })).toHaveAttribute(
+ 'form',
+ 'sink-pubsub-form',
+ );
});Also applies to: 59-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsubModal.spec.tsx`
around lines 4 - 14, Tests currently mock Button to null so they never exercise
the footer->form submit link introduced by the refactor; update the mocks in
SinkPubsubModal.spec.tsx so the Button mock renders an element that
preserves/accepts the form prop (or stop mocking Button entirely) and ensure
ModalFooterWithAlerts mock renders a footer save button with
form="sink-pubsub-form" that can be clicked in the test; then add an assertion
that clicking the footer save button triggers the form submit path (e.g., assert
the form submit handler or the expected post-submit side effect from the Form
component) to cover the new contract between ModalFooterWithAlerts/Button and
the Form.
| const SinkSourceModalProvider: OverlayComponent<Props> = (props) => ( | ||
| <Modal isOpen onClose={props.closeOverlay} variant="small"> | ||
| <SinkSourceController cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </Modal> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find frontend/packages/knative-plugin/src/components/sink-source -type f -name "*.tsx" -o -name "*.ts" | head -20Repository: openshift/console
Length of output: 485
🏁 Script executed:
cat -n frontend/packages/knative-plugin/src/components/sink-source/SinkSourceController.tsx | head -50Repository: openshift/console
Length of output: 1470
🏁 Script executed:
cd frontend/packages/knative-plugin/src/components/sink-source && ls -laRepository: openshift/console
Length of output: 444
🏁 Script executed:
cat -n frontend/packages/knative-plugin/src/components/sink-source/SinkSourceModal.tsxRepository: openshift/console
Length of output: 2907
🏁 Script executed:
cat -n frontend/packages/knative-plugin/src/components/sink-source/SinkSource.tsxRepository: openshift/console
Length of output: 3716
Wire aria-labelledby on the Modal to connect it to the ModalHeader.
The outer Modal component (lines 20-24) lacks an accessible name. Since SinkSourceModal uses a ModalHeader with labelId="sink-source-modal-title", the Modal wrapper must reference this ID via aria-labelledby="sink-source-modal-title" to meet PatternFly's accessibility requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/sink-source/SinkSourceController.tsx`
around lines 20 - 24, The Modal wrapper in SinkSourceModalProvider is missing an
accessible name; update the Modal element in the SinkSourceModalProvider
component to include aria-labelledby="sink-source-modal-title" so it references
the ModalHeader's labelId (used inside SinkSourceController/ModalHeader),
ensuring the Modal and ModalHeader are connected for accessibility.
| jest.mock('@patternfly/react-core', () => ({ | ||
| ...jest.requireActual('@patternfly/react-core'), | ||
| ModalHeader: jest.fn(() => null), | ||
| ModalBody: jest.fn(({ children }) => <div>{children}</div>), | ||
| Button: jest.fn(() => null), | ||
| Form: jest.fn(({ children, ...props }) => <form {...props}>{children}</form>), | ||
| FormGroup: jest.fn(({ children }) => <div>{children}</div>), | ||
| FormHelperText: jest.fn(() => null), | ||
| HelperText: jest.fn(() => null), | ||
| HelperTextItem: jest.fn(() => null), | ||
| TextInputTypes: { | ||
| text: 'text', | ||
| url: 'url', | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('@console/shared/src/components/modals/ModalFooterWithAlerts', () => ({ | ||
| ModalFooterWithAlerts: jest.fn(({ children }) => <div>{children}</div>), | ||
| })); |
There was a problem hiding this comment.
The updated spec is still render-only, so the modal migration is effectively untested.
After moving actions into a PF footer, the risky part is the footer-to-form contract. These assertions only check that rendering does not throw, so a broken submit button or missing form id would still pass.
Also applies to: 58-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/sink-uri/__tests__/SinkUriModal.spec.tsx`
around lines 6 - 24, The current spec only verifies render, so add tests in
SinkUriModal.spec.tsx that exercise the footer-to-form contract: render the
SinkUriModal (or the specific component under test), locate the submit button
rendered via the mocked ModalFooterWithAlerts/Button, assert its form attribute
equals the Form's id (or that the form element has the expected id from the
component), and simulate a click (or fireEvent.submit on the form) to verify the
component's onSubmit/submit handler is invoked; also add a test that ensures a
disabled/missing submit button fails the expected submit behavior. Use the
existing mocked Form, ModalFooterWithAlerts, and Button symbols to find elements
and assert the form-id/form-attribute linkage and actual submit invocation.
| <Modal isOpen onClose={props.closeOverlay} variant="small"> | ||
| <SinkUriController cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </ModalWrapper> | ||
| </Modal> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the SinkUriController.tsx file
find . -type f -name "SinkUriController.tsx" 2>/dev/null | head -5Repository: openshift/console
Length of output: 141
🏁 Script executed:
# Check the SinkUriModal.tsx file too
find . -type f -name "SinkUriModal.tsx" 2>/dev/null | head -5Repository: openshift/console
Length of output: 136
🏁 Script executed:
# Find the knative-plugin directory structure
find . -type d -name "knative-plugin" 2>/dev/null | head -3Repository: openshift/console
Length of output: 95
🏁 Script executed:
cat -n ./frontend/packages/knative-plugin/src/components/sink-uri/SinkUriController.tsx | head -30Repository: openshift/console
Length of output: 1459
🏁 Script executed:
cat -n ./frontend/packages/knative-plugin/src/components/sink-uri/SinkUriModal.tsx | head -50Repository: openshift/console
Length of output: 1858
🏁 Script executed:
# Search for other Modal usage in the codebase to see if aria-labelledby pattern is used elsewhere
rg -A 3 -B 1 '<Modal' ./frontend --type tsx --type ts | grep -A 3 'aria-labelledby\|labelId' | head -40Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Check PatternFly version
cat ./frontend/packages/knative-plugin/package.json 2>/dev/null | grep -A 5 '@patternfly'Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for aria-labelledby usage in Modal context
rg 'aria-labelledby.*modal\|Modal.*aria-labelledby' ./frontend -i | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find package.json for knative plugin
find ./frontend/packages/knative-plugin -name "package.json" -type fRepository: openshift/console
Length of output: 174
🏁 Script executed:
# Look for other Modal patterns with labelId
rg '<Modal' ./frontend -A 5 | grep -B 5 -A 5 'labelId\|aria-label' | head -50Repository: openshift/console
Length of output: 4307
🌐 Web query:
PatternFly React Modal accessibility aria-labelledby best practices
💡 Result:
-
Give the modal an accessible name (required):
- If your modal has a visible title (e.g.,
ModalHeader), setaria-labelledbyonModalto the ID of the title element (PatternFly notes this is required when.pf-v6-c-modal-box__title/a title is present). [1][2] - If there is no visible title, use
aria-labelonModalinstead. [1][2]
- If your modal has a visible title (e.g.,
-
Wire PatternFly React IDs correctly (recommended):
- Set
ModalHeader labelId="my-modal-title"and passaria-labelledby="my-modal-title"toModal(PatternFly React docs explicitly sayaria-labelledbyshould include theModalHeader labelId). [2]
- Set
-
Add a description when it helps (recommended):
- If there’s static content that explains the modal’s purpose, set
aria-describedbyonModalto the ID of that descriptive content (PatternFly React: should matchModalHeader descriptorId; PatternFly accessibility guidance: typically the primary modal content). [1][2]
- If there’s static content that explains the modal’s purpose, set
-
Don’t forget other required a11y bits PatternFly calls out:
- Modal container should have
role="dialog"andaria-modal="true". [1][3] - Close button needs an accessible name (PatternFly uses
aria-label="Close"). [1][3]
- Modal container should have
Example (PatternFly React):
<Modal
isOpen
aria-labelledby="user-edit-title"
aria-describedby="user-edit-desc"
onClose={onClose}
>
<ModalHeader labelId="user-edit-title" descriptorId="user-edit-desc" title="Edit user" />
<ModalBody>...</ModalBody>
</Modal>Sources: PatternFly Modal accessibility guidance [1][3] and PatternFly React Modal prop docs [2].
Wire aria-labelledby on the Modal to connect the title.
The <Modal> component is missing aria-labelledby, even though SinkUriModal provides labelId="sink-uri-modal-title" on the ModalHeader. PatternFly requires the modal to declare its accessible name by referencing the header's ID—without it, screen readers announce the dialog without a title. This is a required accessibility pattern when a visible title is present.
♿ Suggested fix
- <Modal isOpen onClose={props.closeOverlay} variant="small">
+ <Modal
+ isOpen
+ onClose={props.closeOverlay}
+ variant="small"
+ aria-labelledby="sink-uri-modal-title"
+ >📝 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.
| <Modal isOpen onClose={props.closeOverlay} variant="small"> | |
| <SinkUriController cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </ModalWrapper> | |
| </Modal> | |
| <Modal | |
| isOpen | |
| onClose={props.closeOverlay} | |
| variant="small" | |
| aria-labelledby="sink-uri-modal-title" | |
| > | |
| <SinkUriController cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </Modal> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/sink-uri/SinkUriController.tsx`
around lines 23 - 25, The Modal wrapper in SinkUriController is missing the
aria-labelledby attribute that should reference the ModalHeader's ID
("sink-uri-modal-title"); update the Modal element in SinkUriController (the
<Modal> that renders <SinkUriController ... />) to include
aria-labelledby="sink-uri-modal-title" so the dialog's accessible name is tied
to the header ID used by SinkUriModal/ModalHeader.
| <Modal isOpen onClose={props.closeOverlay} variant="small"> | ||
| <TestFunctionController cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </ModalWrapper> | ||
| </Modal> |
There was a problem hiding this comment.
Missing aria-labelledby for modal accessibility.
The TestFunctionModal child component sets labelId="test-function-modal-title" on its ModalHeader, but the parent Modal here doesn't reference it. Screen readers won't associate the modal with its title without this connection.
♿ Proposed fix for accessibility
- <Modal isOpen onClose={props.closeOverlay} variant="small">
+ <Modal isOpen onClose={props.closeOverlay} variant="small" aria-labelledby="test-function-modal-title">📝 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.
| <Modal isOpen onClose={props.closeOverlay} variant="small"> | |
| <TestFunctionController cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </ModalWrapper> | |
| </Modal> | |
| <Modal isOpen onClose={props.closeOverlay} variant="small" aria-labelledby="test-function-modal-title"> | |
| <TestFunctionController cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </Modal> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/test-function/TestFunctionController.tsx`
around lines 42 - 44, The Modal instance wrapping TestFunctionController is
missing an accessibility link to the modal header; add
aria-labelledby="test-function-modal-title" to the Modal props so screen readers
can associate the dialog with the TestFunctionController's ModalHeader (which
uses labelId="test-function-modal-title"); keep existing onClose/cancel props
and other props unchanged.
| <Form | ||
| id="test-function-form" | ||
| onSubmit={() => { | ||
| handleSubmit(); | ||
| setCurrentView(ModalPanel.Response); | ||
| }} | ||
| className="pf-v6-u-mr-md" | ||
| > | ||
| <Button type="submit" variant="primary" data-test="test-action" isDisabled={isSubmitting}> | ||
| {t('knative-plugin~Test')} | ||
| </Button> | ||
| | ||
| <Button type="button" variant="link" data-test="cancel-action" onClick={cancel}> | ||
| {t('knative-plugin~Cancel')} | ||
| </Button> | ||
| </form> | ||
| ) : ( | ||
| <> | ||
| <Button | ||
| type="button" | ||
| variant="primary" | ||
| data-test="back-action" | ||
| onClick={() => { | ||
| clearResponseValues(props); | ||
| setCurrentView(ModalPanel.Request); | ||
| }} | ||
| > | ||
| {t('knative-plugin~Back')} | ||
| </Button> | ||
| | ||
| <Button | ||
| type="button" | ||
| variant="link" | ||
| data-test="close-action" | ||
| onClick={() => { | ||
| clearResponseValues(props); | ||
| close(); | ||
| }} | ||
| > | ||
| {t('knative-plugin~Close')} | ||
| </Button> | ||
| </> | ||
| )} | ||
| </> | ||
| ); | ||
|
|
||
| return ( | ||
| <Modal | ||
| variant={ModalVariant.small} | ||
| isOpen | ||
| header={header} | ||
| className="kn-test-sf-modal" | ||
| onClose={close} | ||
| position="top" | ||
| footer={footer} | ||
| data-test="test-serverless-function" | ||
| > | ||
| <div className="kn-test-sf-modal__body"> | ||
| <> | ||
| {currentView === ModalPanel.Request ? ( | ||
| <RequestPane {...props} /> | ||
| ) : ( | ||
| <ResponsePane {...props} /> | ||
| )} | ||
| </> | ||
| </Form> |
There was a problem hiding this comment.
Form onSubmit handler may cause unexpected page navigation.
The onSubmit wraps Formik's handleSubmit() but doesn't pass the event or call preventDefault(). Formik's handleSubmit only prevents default when it receives the event directly. This could cause a full page reload when submitting.
🐛 Proposed fix to prevent default form submission
<Form
id="test-function-form"
- onSubmit={() => {
- handleSubmit();
+ onSubmit={(e) => {
+ e.preventDefault();
+ handleSubmit();
setCurrentView(ModalPanel.Response);
}}
className="pf-v6-u-mr-md"
>📝 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="test-function-form" | |
| onSubmit={() => { | |
| handleSubmit(); | |
| setCurrentView(ModalPanel.Response); | |
| }} | |
| className="pf-v6-u-mr-md" | |
| > | |
| <Button type="submit" variant="primary" data-test="test-action" isDisabled={isSubmitting}> | |
| {t('knative-plugin~Test')} | |
| </Button> | |
| | |
| <Button type="button" variant="link" data-test="cancel-action" onClick={cancel}> | |
| {t('knative-plugin~Cancel')} | |
| </Button> | |
| </form> | |
| ) : ( | |
| <> | |
| <Button | |
| type="button" | |
| variant="primary" | |
| data-test="back-action" | |
| onClick={() => { | |
| clearResponseValues(props); | |
| setCurrentView(ModalPanel.Request); | |
| }} | |
| > | |
| {t('knative-plugin~Back')} | |
| </Button> | |
| | |
| <Button | |
| type="button" | |
| variant="link" | |
| data-test="close-action" | |
| onClick={() => { | |
| clearResponseValues(props); | |
| close(); | |
| }} | |
| > | |
| {t('knative-plugin~Close')} | |
| </Button> | |
| </> | |
| )} | |
| </> | |
| ); | |
| return ( | |
| <Modal | |
| variant={ModalVariant.small} | |
| isOpen | |
| header={header} | |
| className="kn-test-sf-modal" | |
| onClose={close} | |
| position="top" | |
| footer={footer} | |
| data-test="test-serverless-function" | |
| > | |
| <div className="kn-test-sf-modal__body"> | |
| <> | |
| {currentView === ModalPanel.Request ? ( | |
| <RequestPane {...props} /> | |
| ) : ( | |
| <ResponsePane {...props} /> | |
| )} | |
| </> | |
| </Form> | |
| <Form | |
| id="test-function-form" | |
| onSubmit={(e) => { | |
| e.preventDefault(); | |
| handleSubmit(); | |
| setCurrentView(ModalPanel.Response); | |
| }} | |
| className="pf-v6-u-mr-md" | |
| > | |
| <> | |
| {currentView === ModalPanel.Request ? ( | |
| <RequestPane {...props} /> | |
| ) : ( | |
| <ResponsePane {...props} /> | |
| )} | |
| </> | |
| </Form> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/test-function/TestFunctionModal.tsx`
around lines 34 - 49, The Form onSubmit in TestFunctionModal currently calls
handleSubmit() without the event which can allow a full page reload; update the
Form's onSubmit handler to accept the event parameter, call
event.preventDefault(), and then invoke Formik's handleSubmit with that event
before calling setCurrentView(ModalPanel.Response) (referencing the Form id
"test-function-form", the handleSubmit function and
setCurrentView/ModalPanel.Response symbols) so the default browser submit is
stopped and Formik handles submission correctly.
| jest.mock('@patternfly/react-core', () => ({ | ||
| ...jest.requireActual('@patternfly/react-core'), | ||
| ModalHeader: jest.fn(() => null), | ||
| ModalBody: jest.fn(({ children }) => <div>{children}</div>), | ||
| Button: jest.fn(() => null), | ||
| Form: jest.fn(({ children, ...props }) => <form {...props}>{children}</form>), | ||
| })); | ||
|
|
||
| jest.mock('@console/shared/src/components/modals/ModalFooterWithAlerts', () => ({ | ||
| ModalFooterWithAlerts: jest.fn(({ children }) => <div>{children}</div>), | ||
| })); |
There was a problem hiding this comment.
The submit test bypasses the behavior this refactor changed.
fireEvent.submit(form) proves the form handler works, but it does not prove the footer submit button is actually wired back to traffic-splitting-form. With Button mocked to null, this regression would slip through.
Also applies to: 54-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingModal.spec.tsx`
around lines 10 - 20, The test mock for Button hides the real footer submit
wiring — stop returning null for Button in the jest.mock so the footer submit
button is rendered (or replace the Button mock with one that renders a native
<button> and forwards props like type and onClick); update the test to click the
ModalFooterWithAlerts/Button instance (instead of only calling
fireEvent.submit(form)) to verify the submit button in the footer is actually
wired to the traffic-splitting-form handler (references: mocked Button, Form,
ModalFooterWithAlerts and the traffic-splitting-form submit test that currently
uses fireEvent.submit(form)).
| <Modal isOpen onClose={props.closeOverlay} variant="small"> | ||
| <TrafficSplittingController | ||
| cancel={props.closeOverlay} | ||
| close={props.closeOverlay} | ||
| {...props} | ||
| /> | ||
| </ModalWrapper> | ||
| </Modal> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "TrafficSplitting" --extension tsxRepository: openshift/console
Length of output: 644
🏁 Script executed:
cat frontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingController.tsx | head -80Repository: openshift/console
Length of output: 2501
🏁 Script executed:
cat frontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingModal.tsxRepository: openshift/console
Length of output: 1977
🏁 Script executed:
rg -t tsx "Modal.*aria-labelledby" --max-count=10 -B 2 -A 2Repository: openshift/console
Length of output: 88
🏁 Script executed:
rg -t tsx "ModalHeader" --max-count=15 -B 3 | head -100Repository: openshift/console
Length of output: 88
🏁 Script executed:
rg "aria-labelledby" --type-list | grep -i tsRepository: openshift/console
Length of output: 450
🏁 Script executed:
rg "aria-labelledby" . --max-count=15 -B 2 -A 2Repository: openshift/console
Length of output: 14681
🏁 Script executed:
rg "Modal.*isOpen" . -B 1 -A 5 --max-count=20Repository: openshift/console
Length of output: 36404
Add aria-labelledby to the Modal wrapper.
The outer Modal component is missing the aria-labelledby attribute. TrafficSplittingModal declares labelId="traffic-splitting-modal-title" on ModalHeader, but the wrapper never references it. Per PatternFly's accessibility model, the dialog itself must be labeled, not just the header.
Suggested fix
- <Modal isOpen onClose={props.closeOverlay} variant="small">
+ <Modal
+ isOpen
+ onClose={props.closeOverlay}
+ variant="small"
+ aria-labelledby="traffic-splitting-modal-title"
+ >📝 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.
| <Modal isOpen onClose={props.closeOverlay} variant="small"> | |
| <TrafficSplittingController | |
| cancel={props.closeOverlay} | |
| close={props.closeOverlay} | |
| {...props} | |
| /> | |
| </ModalWrapper> | |
| </Modal> | |
| <Modal | |
| isOpen | |
| onClose={props.closeOverlay} | |
| variant="small" | |
| aria-labelledby="traffic-splitting-modal-title" | |
| > | |
| <TrafficSplittingController | |
| cancel={props.closeOverlay} | |
| close={props.closeOverlay} | |
| {...props} | |
| /> | |
| </Modal> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingController.tsx`
around lines 59 - 65, The Modal wrapper lacks an aria-labelledby that points to
the ModalHeader id; add aria-labelledby="traffic-splitting-modal-title" to the
Modal element that renders TrafficSplittingController so the dialog is properly
labeled (i.e., update the Modal JSX where Modal is opened to include
aria-labelledby="traffic-splitting-modal-title", referencing the existing
labelId on the ModalHeader).
5f0021c to
360866c
Compare
Migrate 7 modals from deprecated @console/internal/components/factory ModalWrapper to modern @patternfly/react-core Modal components: - SinkSourceModal - PubSubModal - SinkPubsubModal - DeleteRevisionModal - SinkUriModal - TestFunctionModal - TrafficSplittingModal Changes: - Replace ModalWrapper with PatternFly 6 Modal in controller components - Replace ModalTitle with ModalHeader (with labelId and data-test-id="modal-title") - Move Form inside ModalBody for consistency - Replace ModalSubmitFooter with ModalFooter or ModalFooterWithAlerts - Add form attribute to submit buttons to connect to form id - Add data-test-id="modal-cancel-action" to cancel buttons for integration tests - Update unit tests to mock PatternFly 6 components - Fix TestFunction bug: service.data.metadata -> service.metadata All modals now follow consistent pattern: - Modal wrapper in Controller component - ModalHeader with accessibility labelId and test ID - Form inside ModalBody with unique id and className="pf-v6-u-mr-md" - Submit button with type="submit" and form attribute - Cancel button with variant="link" and test ID Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4447-knative
branch
from
360866c to
b219911
Compare
|
@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. |
4 participants
Summary
Migrate knative-plugin modals from deprecated factory components to PatternFly 6 Modal components.
This PR migrates 7 modals:
Note: This PR includes changes from #16015, so it should merge first.
Changes
ModalWrapperwith PatternFly 6Modalin controller componentsModalTitlewithModalHeader(withlabelIdanddata-test-id="modal-title")ForminsideModalBodyfor consistencyModalSubmitFooterwithModalFooterorModalFooterWithAlertsformattribute to submit buttons to connect to form iddata-test-id="modal-cancel-action"to cancel buttons for integration testsservice.data.metadata→service.metadataPattern
All modals now follow consistent pattern:
variant="small"labelIdanddata-test-id="modal-title"className="pf-v6-u-mr-md"type="submit"andformattributevariant="link"anddata-test-id="modal-cancel-action"Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Tests