fix: updated with notfication badge with unified theme#8206
fix: updated with notfication badge with unified theme#8206mcoker merged 1 commit intopatternfly:mainfrom
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 (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughUpdated Button SCSS tokens (border-radius and border-color references) and minor whitespace; replaced inline Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
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-8206.surge.sh A11y report: https://pf-pr-8206-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Button/button.scss`:
- Line 710: The line defining the CSS variable "--#{$button}--Color:
var(--#{$button}--m-unread--Color);" has incorrect indentation; update the
leading whitespace in src/patternfly/components/Button/button.scss so this line
is indented to match the surrounding block (lines ~711-718) for consistent
formatting of the --#{$button}--Color declaration.
In `@src/patternfly/components/Button/notification-badge.hbs`:
- Around line 13-19: The icons rendered by the pfIcon helper in
notification-badge.hbs (the three pfIcon calls inside the
notification-badge--IsTask / notification-badge--IsAttention / default branches)
are decorative and must be hidden from AT; either update the pfIcon helper to
accept an optional aria-hidden parameter and emit aria-hidden="true" on the
returned SVG when provided, or wrap each pfIcon invocation in a container with
aria-hidden="true" (e.g., a span) so the rendered SVGs are not announced by
screen readers; apply the chosen approach to all three pfIcon usages in this
template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90cc0e04-a7f8-474e-984a-e29fd285be24
📒 Files selected for processing (2)
src/patternfly/components/Button/button.scsssrc/patternfly/components/Button/notification-badge.hbs
dlabaj
changed the title
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
@nicolethoen I can add another issue for the plain variant if you want to keep these small
This PR looks good to me!
|
@andrew-ronaldson yes thank you! |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Nice! Just a couple of things.
| &.pf-m-read, | ||
| &.pf-m-unread, | ||
| &.pf-m-attention { | ||
| --#{$button}--BorderRadius: var(--pf-t--global--border--radius--action--plain--default); | ||
| } |
There was a problem hiding this comment.
Could you just add this to &.pf-m-stateful, or is there a reason that won't work? Stateful buttons are only used for the notification badge states as far as I'm aware.
Also however you end up doing it, can you create a button var for this style at the top of the CSS file and reference that button var in this block? Like if you applied it to .pf-m-stateful, add the var like this
// at the top of the file with the other vars
--#{$button}--m-stateful--BorderRadius: var(--pf-t--global--border--radius--action--plain--default);
// with the other stateful vars
&.pf-m-stateful {
--#{$button}--BorderRadius: var(--#{$button}--m-stateful--BorderRadius);
}There was a problem hiding this comment.
fixed. Move to just use stateful.
There was a problem hiding this comment.
Are you able to remove .pf-m-read, .pf-m-unread, and .pf-m-attention? A button with .pf-m-stateful should always be used with one of those statuses, so just styling .pf-m-stateful should be all you need.
| // Unread | ||
| &.pf-m-unread { | ||
| --#{$button}--Color: var(--#{$button}--m-unread--Color); | ||
| --#{$button}--Color: var(--#{$button}--m-unread--Color); |
There was a problem hiding this comment.
Just a nit but can you add that space back?
|
I added the plain variant issue |
|
|
||
| // Stateful | ||
| --#{$button}--m-stateful--BorderRadius: var(--pf-t--global--border--radius--small); | ||
| --#{$button}--m-stateful--BorderRadius: var(--pf-t--global--border--radius--action--plain--default); |
There was a problem hiding this comment.
@mcoker Updated to use stateful per your suggestion in the review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/patternfly/components/Button/button.scss (1)
736-742: Redundant nested selectors and inconsistent variable usage.This block appears redundant since line 731 already sets
--#{$button}--BorderRadiusfor all.pf-m-statefulbuttons. Additionally:
- The
&.pf-m-statefulselector (line 740) inside&.pf-m-statefulcreates.pf-m-stateful.pf-m-stateful, which is functionally the same as.pf-m-stateful.- This block uses the token directly instead of referencing the variable
--#{$button}--m-stateful--BorderRadiusdefined at line 239.If this block is intentionally kept for clarity or future extensibility, consider at least removing the redundant
&.pf-m-statefulselector and using the variable for consistency:🔧 Suggested cleanup
&.pf-m-stateful { --#{$button}--BorderRadius: var(--#{$button}--m-stateful--BorderRadius); --#{$button}--PaddingInlineStart: var(--#{$button}--m-stateful--PaddingInlineStart); --#{$button}--PaddingInlineEnd: var(--#{$button}--m-stateful--PaddingInlineEnd); --#{$button}--m-small--PaddingInlineEnd: var(--#{$button}--m-stateful--m-small--PaddingInlineEnd); --#{$button}--m-small--PaddingInlineStart: var(--#{$button}--m-stateful--m-small--PaddingInlineStart); - - &.pf-m-read, - &.pf-m-unread, - &.pf-m-attention, - &.pf-m-stateful { - --#{$button}--BorderRadius: var(--pf-t--global--border--radius--action--plain--default); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Button/button.scss` around lines 736 - 742, Remove the redundant nested selectors and switch to the stateful variable: in the block containing &.pf-m-read, &.pf-m-unread, &.pf-m-attention, &.pf-m-stateful, delete the duplicate nested &.pf-m-stateful selector (which produces .pf-m-stateful.pf-m-stateful) and replace the direct token assignment --#{$button}--BorderRadius with the stateful token variable --#{$button}--m-stateful--BorderRadius so the rule reads something like setting --#{$button}--BorderRadius: var(--#{$button}--m-stateful--BorderRadius) for those modifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/patternfly/components/Button/button.scss`:
- Around line 736-742: Remove the redundant nested selectors and switch to the
stateful variable: in the block containing &.pf-m-read, &.pf-m-unread,
&.pf-m-attention, &.pf-m-stateful, delete the duplicate nested &.pf-m-stateful
selector (which produces .pf-m-stateful.pf-m-stateful) and replace the direct
token assignment --#{$button}--BorderRadius with the stateful token variable
--#{$button}--m-stateful--BorderRadius so the rule reads something like setting
--#{$button}--BorderRadius: var(--#{$button}--m-stateful--BorderRadius) for
those modifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d1753a55-993c-47d1-875b-1bce0a3cac70
📒 Files selected for processing (1)
src/patternfly/components/Button/button.scss
|
@dlabaj this needs a rebase |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
This looks good but looks like you have a conflict. Good news is I think it's just from something prettier changed.
Can you undo the prettier changes? Sorry about that, I don't think any of the core devs use prettier so we don't get those updates. Related, I put up a PR to disable prettier here - #8257
dlabaj
force-pushed
the
notification-update
branch
from
cf79ab3 to
b1aaff6
Compare
|
🎉 This PR is included in version 6.5.0-prerelease.63 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
Updated with notification badge changes per issue #8131
Summary by CodeRabbit
Style
Refactor