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

thatblindgeye · 2025-05-28T14:20:35Z

What: Closes #11850

Additional issues:

patternfly-build · 2025-05-28T14:24:42Z

Preview: https://patternfly-react-pr-11851.surge.sh

A11y report: https://patternfly-react-pr-11851-a11y.surge.sh

thatblindgeye · 2025-05-28T18:23:41Z

First commit shows the original logic where a new pf-m-detached class was added by default when the pre-existing isDetached prop was passed in.

Second commit is more true opt-in where isDetached doesn't do anything new unless hasDetachedAnimations is also passed in. Note that I didn't update the direction prop and also didnt remove the pf-m-detached class from the Toggle component file; both of those I can update if this is the route we wanna go with for now

kmcfaul · 2025-05-29T15:27:00Z

packages/react-core/src/components/ExpandableSection/ExpandableSectionToggle.tsx

@@ -63,7 +67,7 @@ export const ExpandableSectionToggle: React.FunctionComponent<ExpandableSectionT
            <span
              className={css(
                styles.expandableSectionToggleIcon,
-                isExpanded && direction === 'up' && styles.modifiers.expandTop
+                isExpanded && direction === 'up' && styles.modifiers.expandTop // TODO: next breaking change move this class to the outter styles.expandableSection wrapper


Outer misspelled, and is there an issue open for this?

kmcfaul · 2025-05-29T15:32:57Z

packages/react-core/src/components/ExpandableSection/examples/ExpandableSection.md

@@ -32,6 +32,8 @@ import CheckCircleIcon from '@patternfly/react-icons/dist/esm/icons/check-circle

 When passing the `isDetached` property into `<ExpandableSection>`, you must also manually pass in the same `toggleId` and `contentId` properties to both `<ExpandableSection>` and `<ExpandableSectionToggle>`. This will link the content to the toggle via ARIA attributes.

+By default animations will not be enabled for a detached `<ExpandableSection>`. You must manually pass the `direction` property with an appropriate value based on where the expandable content is rendered. If the expandable content is above the expandable toggle, `direction="up"` must be passed like in this example.


I don't mind the hasDetachedAnimations approach if we want to be more true to opt-in, but my main concern is that with how core is currently set up animations will be on by default and be potentially the incorrect direction for detached sections until updated (because pf-m-detached is what is disabling animations from detached and that won't be present until the flag is enabled).

I wonder if it would be better to make animations as a whole opt-in for expandable section with a new modifier instead, and as part of that also require the direction to be passed in for detached?

cc @mcoker wdyt?

thatblindgeye · 2025-06-05T12:40:41Z

Latest commit reflects convo during retroplanning yesterday where we decided to try updating the Core PR to target the pf-v6-c-expandable-section when it only had a single child, which negates the need of that pf-m-detached modifier class. The plan would be to add that class in in the next breaking change. The latest Core updates can be tested against this PR locally.

mcoker

L🦦TM!

thatblindgeye · 2025-06-16T16:03:48Z

packages/react-core/src/components/ExpandableSection/ExpandableSectionToggle.tsx

@@ -28,6 +28,8 @@ export interface ExpandableSectionToggleProps extends Omit<React.HTMLProps<HTMLD
  isExpanded?: boolean;
  /** Callback function to toggle the expandable content. */
  onToggle?: (isExpanded: boolean) => void;
+  /** Flag indicating that the expandable section and expandable toggle are detached from one another. */
+  isDetached?: boolean;


Per convo with Coker, adding the pf-m-detached classes in both ExpandableSection components but making them opt-in for now. Inn a breaking change we'd want to apply the class by default for the toggle here and apply it only when isDetached is applied to ExpandableSection (will open a followup for this)

rebeccaalpert

I'm reaching for straws here since this looks good to me, but just one nit you're welcome to ignore.

rebeccaalpert · 2025-06-16T20:18:52Z

packages/react-core/src/components/ExpandableSection/ExpandableSectionToggle.tsx

  ...props
 }: ExpandableSectionToggleProps) => (
  <div
    className={css(
      styles.expandableSection,
      isExpanded && styles.modifiers.expanded,
      hasTruncatedContent && styles.modifiers.truncate,
+      isDetached && 'pf-m-detached',


Do we want styles.modifiers.detached instead? Just a nit though.

Ah so right now that token doesn't exist yet since the class isn't being used in the CSS. If we update the CSS in the next breaking change to target this class specifically (rather than right now we're targeting based on the content being the only child), we should be able to use the styles.modifier.detached

patternfly-build · 2025-06-17T13:48:49Z

Your changes have been released in:

@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]

Thanks for your contribution! 🎉

thatblindgeye mentioned this pull request May 28, 2025

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

Merged

kmcfaul reviewed May 29, 2025

View reviewed changes

thatblindgeye force-pushed the expSecDetachedAnims branch from 7317001 to c458f96 Compare June 5, 2025 12:39

thatblindgeye mentioned this pull request Jun 9, 2025

ExpandableSection - move placement of icon pf-m-expand-top modifier class #11863

Open

thatblindgeye requested a review from mcoker June 11, 2025 21:08

thatblindgeye added 5 commits June 13, 2025 14:05

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

ff16d31

Test for more opt-in functionality

2e8bda6

Removed detached class per Core updates

07308f2

Fixed typo

5fcbc65

Updated hamburgericon class to styles object

300bfd7

thatblindgeye force-pushed the expSecDetachedAnims branch from 4af8ab7 to 300bfd7 Compare June 16, 2025 12:09

thatblindgeye requested review from a team, rebeccaalpert and wise-king-sullyman and removed request for a team June 16, 2025 12:14

mcoker approved these changes Jun 16, 2025

View reviewed changes

Bumped core version, feedback from Coker

a228281

thatblindgeye commented Jun 16, 2025

View reviewed changes

thatblindgeye marked this pull request as ready for review June 16, 2025 16:04

rebeccaalpert approved these changes Jun 16, 2025

View reviewed changes

nicolethoen approved these changes Jun 17, 2025

View reviewed changes

kmcfaul approved these changes Jun 17, 2025

View reviewed changes

nicolethoen merged commit 64befbe into patternfly:main Jun 17, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

thatblindgeye commented May 28, 2025

Uh oh!

patternfly-build commented May 28, 2025 •

edited

Loading

Uh oh!

thatblindgeye commented May 28, 2025

Uh oh!

kmcfaul May 29, 2025

Uh oh!

thatblindgeye Jun 9, 2025

Uh oh!

kmcfaul May 29, 2025 •

edited

Loading

Uh oh!

thatblindgeye Jun 3, 2025

Uh oh!

thatblindgeye commented Jun 5, 2025

Uh oh!

mcoker left a comment

Uh oh!

thatblindgeye Jun 16, 2025 •

edited

Loading

Uh oh!

rebeccaalpert left a comment

Uh oh!

rebeccaalpert Jun 16, 2025

Uh oh!

thatblindgeye Jun 16, 2025

Uh oh!

Uh oh!

patternfly-build commented Jun 17, 2025

Uh oh!

Uh oh!

		@@ -32,6 +32,8 @@ import CheckCircleIcon from '@patternfly/react-icons/dist/esm/icons/check-circle

		When passing the `isDetached` property into `<ExpandableSection>`, you must also manually pass in the same `toggleId` and `contentId` properties to both `<ExpandableSection>` and `<ExpandableSectionToggle>`. This will link the content to the toggle via ARIA attributes.

		By default animations will not be enabled for a detached `<ExpandableSection>`. You must manually pass the `direction` property with an appropriate value based on where the expandable content is rendered. If the expandable content is above the expandable toggle, `direction="up"` must be passed like in this example.

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

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

Conversation

thatblindgeye commented May 28, 2025

Uh oh!

patternfly-build commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye commented May 28, 2025

Uh oh!

kmcfaul May 29, 2025

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

kmcfaul May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

thatblindgeye commented Jun 5, 2025

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebeccaalpert left a comment

Choose a reason for hiding this comment

Uh oh!

rebeccaalpert Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patternfly-build commented Jun 17, 2025

Uh oh!

Uh oh!

patternfly-build commented May 28, 2025 •

edited

Loading

kmcfaul May 29, 2025 •

edited

Loading

thatblindgeye Jun 16, 2025 •

edited

Loading