Skip to content

fix(ExpandableSection): made animation opt-in for detached variant#7550

Merged
mcoker merged 3 commits intopatternfly:mainfrom
thatblindgeye:iss7515_expSecDetachedAnims
Jun 16, 2025
Merged

fix(ExpandableSection): made animation opt-in for detached variant#7550
mcoker merged 3 commits intopatternfly:mainfrom
thatblindgeye:iss7515_expSecDetachedAnims

Conversation

@thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented May 28, 2025

Closes #7515

Some notes:

  • In React I believe we're fine to add things outside of breaking changes -- wasn't sure if we wanted to apply the pf-m-detached modifier to the pf-v6-c-expandable-section wrapper that only contains the toggle, but I did that here which should be fine to add into React + sorta seemed odd that only one of the wrapper would have that modifier Per discussion in retroplanning 6/4/2025, React won't have the detached class as it won't be needed for animations. Right now it's kept in Core though not marked as required; the plan is in the next breaking change to have those classes applied+required for detached variants
  • Updated docs to mention the new method of changing the toggle icon rotation direction
  • Removes the fade and slide animation for detached variants if neither the top or bottom modifiers are included

@patternfly-build
Copy link
Collaborator

patternfly-build commented May 28, 2025

@thatblindgeye
Copy link
Contributor Author

Can also test how it works in React by using this draft PR patternfly/patternfly-react#11851

@thatblindgeye thatblindgeye force-pushed the iss7515_expSecDetachedAnims branch from 24dc37b to db9cd23 Compare June 5, 2025 12:34
Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor nits in docs. Looks good to me otterwise. 🦦

transform: rotate(var(--#{$expandable-section}__toggle-icon--Rotate));

&.pf-m-expand-top {
&.pf-m-expand-top { // TODO: Remove this block in breaking change in favor of using modifier on outter expandable section wrapper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&.pf-m-expand-top { // TODO: Remove this block in breaking change in favor of using modifier on outter expandable section wrapper
&.pf-m-expand-top { // TODO: Remove this block in breaking change in favor of using modifier on outer expandable section wrapper

😛 🦦

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You and Katie calling me outt 😭 (that misspelling was intentional at least)

It's funny because I think I told Katie the same thing about maybe having otter's on the mind 😆

--#{$expandable-section}__toggle-icon--Rotate: var(--#{$expandable-section}__toggle-icon--m-expand-top--Rotate);
}

&:has(.#{$expandable-section}__content:only-child) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here about the assumption we're making about the structure with the only-child?

| `.pf-m-indented` | `.pf-v6-c-expandable-section` | Indicates that the expandable section content is indented and is aligned with the start of the title text to provide visual hierarchy. |
| `.pf-m-truncate` | `.pf-v6-c-expandable-section` | Indicates that the expandable section content is truncated by default, and not truncated when expanded. |
| `.pf-m-expand-top` | `.pf-v6-c-expandable-section__toggle-icon` | Modifies the toggle icon to point up when expanded. |
| `.pf-m-expand-top` | `.pf-v6-c-expandable-section__toggle-icon` | Modifies the toggle icon to point up when expanded. We recommend the new method of applying this class directly to the `.pf-v6-c-expandable-section` wrapper element. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt about moving this adjacent to the other .pf-m-expand-top so that a reader would see them together? I just wonder if it could be confusing if one is missed.

@thatblindgeye thatblindgeye force-pushed the iss7515_expSecDetachedAnims branch from db9cd23 to 0ace978 Compare June 13, 2025 12:37
Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super, thank you!

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@mcoker mcoker merged commit 47273b0 into patternfly:main Jun 16, 2025
4 checks passed
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.3.0-prerelease.33 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug - Expandable section - detached animation direction

5 participants