feat(Accordion): unified theme updates#8217
Conversation
WalkthroughThis PR updates the accordion component's SCSS styling to introduce new CSS custom properties for expanded toggle appearance, refactor expandable content margins and background color references, adjust large-display toggle spacing, and scope hover/focus behavior to non-expanded items. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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. 📝 Coding Plan for PR comments
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 |
|
|
||
| // accordion item | ||
| --#{$accordion}__item--BorderRadius: var(--pf-t--global--border--radius--200); | ||
| --#{$accordion}__item--m-expanded--BackgroundColor: var(--pf-t--global--background--color--action--plain--clicked); // TODO - remove "m-expanded" in breaking change |
There was a problem hiding this comment.
This item--m-expanded--BackgroundColor variable is no longer in use with the PR updates. I can either leave it as is, remove it, or refactor the PR to use this variable instead of the newly added --#{$accordion}__toggle--m-expanded--BackgroundColor which I added to replace it (since now only the toggle's background color changes).
|
Preview: https://pf-pr-8217.surge.sh A11y report: https://pf-pr-8217-a11y.surge.sh |
| --#{$accordion}__expandable-content--MarginInlineStart: var(--pf-t--global--spacer--md); | ||
| --#{$accordion}__expandable-content--BackgroundColor: var(--pf-t--global--background--color--primary--default); | ||
| --#{$accordion}__expandable-content--MarginInlineStart: var(--pf-t--global--spacer--lg); | ||
| --#{$accordion}__expandable-content--BackgroundColor: var(--#{$accordion}--BackgroundColor); |
There was a problem hiding this comment.
This expandable-content--BackgroundColor variable may no longer be necessary since the background color of the expandable content should now match the overall background color. Should I remove the background styling entirely here?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/patternfly/components/Accordion/accordion.scss (1)
251-262: Consider addingmargin-block-startto transition-property.The hidden state now resets
margin-block-startto 0 (line 252), and the visible state uses the token value (line 262). However, line 274'stransition-propertyonly includesmargin-block-end, notmargin-block-start. If smooth animated transitions for the top margin are intended, consider addingmargin-block-startto the transition properties.If the instant change is intentional (e.g., to prevent visual artifacts during expand/collapse), this is fine as-is.
♻️ Optional: Add margin-block-start to transitions
- transition-property: opacity, translate, visibility, max-height, margin-block-end; + transition-property: opacity, translate, visibility, max-height, margin-block-start, margin-block-end;Note: You'd also need to add corresponding delay and duration values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Accordion/accordion.scss` around lines 251 - 262, The transition currently only includes margin-block-end but the hidden state resets --#{$accordion}__expandable-content--MarginBlockStart while the visible state uses it, so update the transition-property on .#{$accordion}__expandable-content to include margin-block-start (alongside margin-block-end) and ensure you add corresponding transition-duration/-delay tokens or values consistent with the existing transition for margin-block-end; target the CSS selector .#{$accordion}__expandable-content (and the hidden selector .#{$accordion}__expandable-content:where([hidden])) and reference the token --#{$accordion}__expandable-content--MarginBlockStart when adding the transition so top-margin changes animate smoothly.
🤖 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/Accordion/accordion.scss`:
- Around line 251-262: The transition currently only includes margin-block-end
but the hidden state resets
--#{$accordion}__expandable-content--MarginBlockStart while the visible state
uses it, so update the transition-property on .#{$accordion}__expandable-content
to include margin-block-start (alongside margin-block-end) and ensure you add
corresponding transition-duration/-delay tokens or values consistent with the
existing transition for margin-block-end; target the CSS selector
.#{$accordion}__expandable-content (and the hidden selector
.#{$accordion}__expandable-content:where([hidden])) and reference the token
--#{$accordion}__expandable-content--MarginBlockStart when adding the transition
so top-margin changes animate smoothly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a234ecbb-408e-4143-aa0e-fe49acd27c7f
📒 Files selected for processing (1)
src/patternfly/components/Accordion/accordion.scss
Closes #8079
Summary:
Summary by CodeRabbit
Style