feat(Accordion): unified theme updates#8217
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)
WalkthroughAdded and remapped accordion CSS custom properties: new expanded-state toggle/background/border tokens, updated expandable-content spacing/background/transitions, display-lg padding tweak, narrowed hover/focus scope, and removal of some exported expandable-content-body padding tokens. Root and expandable-content backgrounds set to transparent. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
|
Preview: https://pf-pr-8217.surge.sh A11y report: https://pf-pr-8217-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/patternfly/components/Accordion/accordion.scss (1)
11-11:⚠️ Potential issue | 🟠 MajorPreserve backward compatibility for the legacy expanded background token.
--#{$accordion}__item--m-expanded--BackgroundColoris still exported on Line 11, but it no longer drives rendered styles after this refactor. That can silently break existing downstream overrides that still set this public token. Please alias the new expanded-toggle token to the legacy one until the breaking-change window.Suggested compatibility-safe mapping
- --#{$accordion}__item--m-expanded__toggle--BackgroundColor: var(--pf-t--global--background--color--action--plain--clicked); + --#{$accordion}__item--m-expanded__toggle--BackgroundColor: var(--#{$accordion}__item--m-expanded--BackgroundColor);Also applies to: 21-21, 234-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Accordion/accordion.scss` at line 11, The legacy token --#{$accordion}__item--m-expanded--BackgroundColor is still exported but no longer drives rendered styles; reintroduce a compatibility alias by assigning that legacy custom property to the new expanded-toggle token value (i.e. set --#{$accordion}__item--m-expanded--BackgroundColor: var(--#{$accordion}__item--<expanded-toggle-token-name>--BackgroundColor)) so downstream overrides continue to work, and apply the same aliasing approach for the other affected legacy tokens referenced in the diff (ensure you update the corresponding token names where the new expanded-toggle token is used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/patternfly/components/Accordion/accordion.scss`:
- Line 11: The legacy token --#{$accordion}__item--m-expanded--BackgroundColor
is still exported but no longer drives rendered styles; reintroduce a
compatibility alias by assigning that legacy custom property to the new
expanded-toggle token value (i.e. set
--#{$accordion}__item--m-expanded--BackgroundColor:
var(--#{$accordion}__item--<expanded-toggle-token-name>--BackgroundColor)) so
downstream overrides continue to work, and apply the same aliasing approach for
the other affected legacy tokens referenced in the diff (ensure you update the
corresponding token names where the new expanded-toggle token is used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 774e6975-d05e-470f-92c2-56ae658cefe7
📒 Files selected for processing (1)
src/patternfly/components/Accordion/accordion.scss
|
After chatting with @lboehling and @andrew-ronaldson, going to close this in favor of what was included in the glass work where the original accordion designs are preserved in non-glass theme, and glass will apply plain styles. |
4 participants
Closes #8079
Summary:
Summary by CodeRabbit