chore(patterns): fix status labels in dashboard demo - #8455
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughTwo card demo Handlebars templates are updated to migrate from color-based labels ( ChangesLabel color-to-status migration in Card templates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview: https://pf-pr-8455.surge.sh A11y report: https://pf-pr-8455-a11y.surge.sh |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs`:
- Around line 53-67: The logic in the if-else block for
card-template-expandable-status-card--RequiresReboot is inverted. Currently,
when the condition is truthy, it displays "is not required" with a success icon,
and when falsy, it displays "is required" with a danger icon. Swap the content
of the {{`#if`}} and {{else}} branches so that when RequiresReboot is true, it
shows "is required" with the danger icon, and when RequiresReboot is false, it
shows "is not required" with the success icon. This will align the UI behavior
with the semantic meaning of the flag name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b111bfcb-a994-4f5e-aaa0-ca0ba3d4eb81
📒 Files selected for processing (2)
src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbssrc/patternfly/demos/Card/templates/card-template-status.hbs
| {{#if card-template-expandable-status-card--RequiresReboot}} | ||
| {{#> icon}} | ||
| {{#> icon-content icon-content--modifier="pf-m-danger"}} | ||
| {{#> icon-content icon-content--modifier="pf-m-success"}} | ||
| {{pfIcon "rh-ui-connected"}} | ||
| {{/icon-content}} | ||
| {{/icon}} | ||
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is</b> required</p> | ||
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is not</b> required</p> | ||
| {{else}} | ||
| {{#> icon}} | ||
| {{#> icon-content icon-content--modifier="pf-m-success"}} | ||
| {{#> icon-content icon-content--modifier="pf-m-danger"}} | ||
| {{pfIcon "rh-ui-connected"}} | ||
| {{/icon-content}} | ||
| {{/icon}} | ||
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is not</b> required</p> | ||
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is </b> required</p> | ||
| {{/if}} |
There was a problem hiding this comment.
RequiresReboot condition currently renders the opposite semantic.
Line 53 shows “is not required” when card-template-expandable-status-card--RequiresReboot is truthy, while Line 66 shows “is required” in the falsy branch. That contradicts the flag name and can invert real boolean-driven UI behavior.
💡 Suggested fix
- {{`#if` card-template-expandable-status-card--RequiresReboot}}
+ {{`#if` card-template-expandable-status-card--RequiresReboot}}
{{#> icon}}
- {{#> icon-content icon-content--modifier="pf-m-success"}}
+ {{#> icon-content icon-content--modifier="pf-m-danger"}}
{{pfIcon "rh-ui-connected"}}
{{/icon-content}}
{{/icon}}
- <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is not</b> required</p>
+ <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is</b> required</p>
{{else}}
{{#> icon}}
- {{#> icon-content icon-content--modifier="pf-m-danger"}}
+ {{#> icon-content icon-content--modifier="pf-m-success"}}
{{pfIcon "rh-ui-connected"}}
{{/icon-content}}
{{/icon}}
- <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is </b> required</p>
+ <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is not</b> required</p>
{{/if}}📝 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.
| {{#if card-template-expandable-status-card--RequiresReboot}} | |
| {{#> icon}} | |
| {{#> icon-content icon-content--modifier="pf-m-danger"}} | |
| {{#> icon-content icon-content--modifier="pf-m-success"}} | |
| {{pfIcon "rh-ui-connected"}} | |
| {{/icon-content}} | |
| {{/icon}} | |
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is</b> required</p> | |
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is not</b> required</p> | |
| {{else}} | |
| {{#> icon}} | |
| {{#> icon-content icon-content--modifier="pf-m-success"}} | |
| {{#> icon-content icon-content--modifier="pf-m-danger"}} | |
| {{pfIcon "rh-ui-connected"}} | |
| {{/icon-content}} | |
| {{/icon}} | |
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is not</b> required</p> | |
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is </b> required</p> | |
| {{/if}} | |
| {{`#if` card-template-expandable-status-card--RequiresReboot}} | |
| {{#> icon}} | |
| {{#> icon-content icon-content--modifier="pf-m-danger"}} | |
| {{pfIcon "rh-ui-connected"}} | |
| {{/icon-content}} | |
| {{/icon}} | |
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is</b> required</p> | |
| {{else}} | |
| {{#> icon}} | |
| {{#> icon-content icon-content--modifier="pf-m-success"}} | |
| {{pfIcon "rh-ui-connected"}} | |
| {{/icon-content}} | |
| {{/icon}} | |
| <p class="{{pfv 'u'}}color-200">System reboot <b class="{{pfv 'u'}}color-100">is not</b> required</p> | |
| {{/if}} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs`
around lines 53 - 67, The logic in the if-else block for
card-template-expandable-status-card--RequiresReboot is inverted. Currently,
when the condition is truthy, it displays "is not required" with a success icon,
and when falsy, it displays "is required" with a danger icon. Swap the content
of the {{`#if`}} and {{else}} branches so that when RequiresReboot is true, it
shows "is required" with the danger icon, and when RequiresReboot is false, it
shows "is not required" with the success icon. This will align the UI behavior
with the semantic meaning of the flag name.
Broken conditional in the cards
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs`:
- Around line 53-67: The conditional in the template checks for
`card-template-expandable-status-card--HasIncident`, but the parent template
invokes the second card instance with `RequiresReboot=true` instead, causing the
condition to evaluate as false and display "is not required" when it should show
"is required". Fix by either restoring the conditional to check `RequiresReboot`
instead of `HasIncident`, or updating the parent template's second card
invocation to pass `HasIncident=true` instead of `RequiresReboot=true`. Either
approach will align the variable being checked with the variable being passed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d805221d-0226-4621-9bb4-3444b4732642
📒 Files selected for processing (1)
src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs`:
- Around line 53-67: The conditional in the template checks for
`card-template-expandable-status-card--HasIncident`, but the parent template
invokes the second card instance with `RequiresReboot=true` instead, causing the
condition to evaluate as false and display "is not required" when it should show
"is required". Fix by either restoring the conditional to check `RequiresReboot`
instead of `HasIncident`, or updating the parent template's second card
invocation to pass `HasIncident=true` instead of `RequiresReboot=true`. Either
approach will align the variable being checked with the variable being passed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d805221d-0226-4621-9bb4-3444b4732642
📒 Files selected for processing (1)
src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs
🛑 Comments failed to post (1)
src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs (1)
53-67:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Verify how the parent template invokes card-template-expandable-status-card # Search for all invocations of card-template-expandable-status-card in the parent template rg -A 10 'card-template-expandable-status-card' src/patternfly/demos/Card/templates/card-template-expandable-status.hbsRepository: patternfly/patternfly
Length of output: 1538
Condition variable mismatch causes incorrect reboot status for the second card.
The conditional was changed to check
card-template-expandable-status-card--HasIncident, but the parent template (card-template-expandable-status.hbs) invokes the second card instance withRequiresReboot=trueand noHasIncidentparameter. SinceHasIncidentis undefined, it evaluates to falsy, entering the else block and displaying "System reboot is not required" even though the card should display "is required".Fix by either:
- Restoring the condition to check
RequiresReboot, or- Updating the second card invocation to pass
HasIncident=trueinstead ofRequiresReboot=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patternfly/demos/Card/templates/card-template-expandable-status-card.hbs` around lines 53 - 67, The conditional in the template checks for `card-template-expandable-status-card--HasIncident`, but the parent template invokes the second card instance with `RequiresReboot=true` instead, causing the condition to evaluate as false and display "is not required" when it should show "is required". Fix by either restoring the conditional to check `RequiresReboot` instead of `HasIncident`, or updating the parent template's second card invocation to pass `HasIncident=true` instead of `RequiresReboot=true`. Either approach will align the variable being checked with the variable being passed.
Conditions were giving errors so streamlined the process.
|
🎉 This PR is included in version 6.6.0-prerelease.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This issue has been resolved in version 6.6.0-prerelease.18 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR addresses a comment from the 6.5 Staging review where the non-status labels are used for statuses.
The labels on core don't seem to be using the new new icons so that should be done in a separate issue.
Note: This PR was vibe coded with Cursor.
Summary by CodeRabbit