Conversation
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
4436ab3 to
7e05284
Compare
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1623 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1623", name: che-dashboard}]}}]" |
WizardNavItem defaults to component="button", which is semantically wrong for non-interactive progress indicators. The steps already had pointer-events: none via CSS, but the button role and element type remained, causing each step to be reported as a button in the DOM. Replace WizardNavItem with plain <li>/<span> elements that apply PatternFly's wizard nav classes directly (pf-v6-c-wizard__nav-link, pf-m-current, pf-m-disabled). Visual output is unchanged. Fixes: https://redhat.atlassian.net/browse/CRW-10924 Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1623 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1623", name: che-dashboard}]}}]" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1623 +/- ##
==========================================
- Coverage 92.43% 92.43% -0.01%
==========================================
Files 596 596
Lines 61158 61162 +4
Branches 4730 4733 +3
==========================================
+ Hits 56530 56533 +3
- Misses 4569 4570 +1
Partials 59 59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
There was a problem hiding this comment.
Review Summary
This is a clean, well-motivated accessibility fix that correctly addresses the semantic HTML issue. The PR replaces PatternFly's WizardNavItem (which renders interactive <button> elements) with plain <li>/<span> elements for display-only progress indicators.
Verdict: Approve with minor suggestions
OUIA test attributes removed - confirm with QE
The previous implementation rendered data-ouia-component-id and data-ouia-component-type attributes via WizardNavItem. If any automated UI testing framework (e.g., Cypress, Selenium suites, or OpenShift CI QE tests) depends on these OUIA selectors to locate wizard step elements, those tests will break.
Does it make sense to confirm with QE that no external tests rely on these attributes before merging?
Positive Feedback
- Excellent PR description with screenshot and manual testing steps
- Using
@patternfly/react-stylesCSS tokens rather than hardcoded class strings will track PatternFly updates automatically - The
navLinkClassName()helper is clean and avoids duplicating the class construction logic - Removal of the no-op
handleGoToStepById()method shows thoughtful curation - The
aria-currentattribute correctly omits itself on non-current steps rather than renderingaria-current={false}- semantically better - Overall the change is well-scoped, focused, and achieves the accessibility goal with minimal code
Review generated by Claude Code with ok-pr-review
|
Additional code-specific feedback:
|
|
Looks good for me with quay.io/eclipse/che-dashboard:pr-1623. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olexii4, olkornii The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
What does this PR do?
Fixes workspace creation progress steps rendering as
<button>elements instead of non-interactive labels.PatternFly's
WizardNavItemdefaults tocomponent="button", which is appropriate for navigable wizard steps but wrong for progress indicators that can't be clicked. The steps on the "Create Workspace" loader page (Initializing, Fetching devfile, etc.) are display-only — they already hadpointer-events: noneapplied via CSS, but the underlying elements were still<button>with the associated role and class.The fix replaces
WizardNavItemwith plain<li>/<span>elements that apply PatternFly's wizard nav CSS classes directly (pf-v6-c-wizard__nav-item,pf-v6-c-wizard__nav-link,pf-m-current,pf-m-disabled). The visual output is identical, but the elements are now semantically correct non-interactive labels.Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes https://redhat.atlassian.net/browse/CRW-10924
Is it tested? How?
<span>, not a<button>, and has nobuttonrole.pf-m-currenthighlight and inactive steps are visually unchanged.