Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
"tslib": "^2.8.1"
},
"devDependencies": {
"@patternfly/patternfly": "6.3.0-prerelease.29",
"@patternfly/patternfly": "6.3.0-prerelease.33",
"case-anything": "^3.1.2",
"css": "^3.0.0",
"fs-extra": "^11.3.0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { css } from '@patternfly/react-styles';
// Because this is such a specific icon that requires being wrapped in a pf-v[current version]-c-button element,
// we don't want to export this to consumers nor include it in the react-icons package as a custom icon.
export const hamburgerIcon = (
<svg viewBox="0 0 10 10" className="pf-v6-c-button--hamburger-icon pf-v6-svg" width="1em" height="1em">
<svg viewBox="0 0 10 10" className={css(styles.buttonHamburgerIcon, 'pf-v6-svg')} width="1em" height="1em">
<path className={css(styles.buttonHamburgerIconTop)} d="M1,1 L9,1"></path>
<path className={css(styles.buttonHamburgerIconMiddle)} d="M1,5 L9,5"></path>
<path className={css(styles.buttonHamburgerIconArrow)} d="M1,5 L1,5 L1,5"></path>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export interface ExpandableSectionProps extends Omit<React.HTMLProps<HTMLDivElem
toggleId?: string;
/** Display size variant. Set to "lg" for disclosure styling. */
displaySize?: 'default' | 'lg';
/** Indicates the expandable section has a detached toggle. */
/** Flag indicating that the expandable section and expandable toggle are detached from one another. */
isDetached?: boolean;
/** Flag to indicate if the content is expanded. */
isExpanded?: boolean;
Expand Down Expand Up @@ -64,6 +64,10 @@ export interface ExpandableSectionProps extends Omit<React.HTMLProps<HTMLDivElem
* variant, the expandable content will be truncated after 3 lines by default.
*/
variant?: 'default' | 'truncate';
/** Sets the direction of the expandable animation when isDetached is true. If this prop is not passed,
* animation will not occur.
*/
direction?: 'up' | 'down';
}

interface ExpandableSectionState {
Expand All @@ -72,6 +76,11 @@ interface ExpandableSectionState {
previousWidth: number;
}

const directionClassMap = {
up: styles.modifiers.expandTop,
down: styles.modifiers.expandBottom
};

const setLineClamp = (lines: number, element: HTMLDivElement) => {
if (!element || lines < 1) {
return;
Expand Down Expand Up @@ -198,6 +207,7 @@ class ExpandableSection extends Component<ExpandableSectionProps, ExpandableSect
variant,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
truncateMaxLines,
direction,
...props
} = this.props;

Expand Down Expand Up @@ -258,6 +268,8 @@ class ExpandableSection extends Component<ExpandableSectionProps, ExpandableSect
displaySize === 'lg' && styles.modifiers.displayLg,
isWidthLimited && styles.modifiers.limitWidth,
isIndented && styles.modifiers.indented,
isDetached && direction && directionClassMap[direction],
isDetached && direction && 'pf-m-detached',
variant === ExpandableSectionVariant.truncate && styles.modifiers.truncate,
className
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

@thatblindgeye thatblindgeye Jun 16, 2025

Choose a reason for hiding this comment

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

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)

}

export const ExpandableSectionToggle: React.FunctionComponent<ExpandableSectionToggleProps> = ({
Expand All @@ -39,13 +41,15 @@ export const ExpandableSectionToggle: React.FunctionComponent<ExpandableSectionT
toggleId,
direction = 'down',
hasTruncatedContent = false,
isDetached,
...props
}: ExpandableSectionToggleProps) => (
<div
className={css(
styles.expandableSection,
isExpanded && styles.modifiers.expanded,
hasTruncatedContent && styles.modifiers.truncate,
isDetached && 'pf-m-detached',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

className
)}
{...props}
Expand All @@ -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 outer styles.expandableSection wrapper
)}
>
<AngleRightIcon />
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { Fragment } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import { ExpandableSection, ExpandableSectionVariant } from '../ExpandableSection';
import { ExpandableSectionToggle } from '../ExpandableSectionToggle';
import styles from '@patternfly/react-styles/css/components/ExpandableSection/expandable-section';

const props = { contentId: 'content-id', toggleId: 'toggle-id' };

Expand All @@ -22,7 +21,7 @@ test('Renders ExpandableSection expanded', () => {
expect(asFragment()).toMatchSnapshot();
});

test('ExpandableSection onToggle called', async () => {
test('Calls onToggle when clicked', async () => {
const mockfn = jest.fn();
const user = userEvent.setup();

Expand All @@ -32,6 +31,21 @@ test('ExpandableSection onToggle called', async () => {
expect(mockfn.mock.calls).toHaveLength(1);
});

test('Does not call onToggle when not clicked', async () => {
const mockfn = jest.fn();
const user = userEvent.setup();

render(
<>
<ExpandableSection onToggle={mockfn}> test </ExpandableSection>
<button onClick={() => {}}>Test clicker</button>
</>
);

await user.click(screen.getByRole('button', { name: 'Test clicker' }));
expect(mockfn).not.toHaveBeenCalled();
});

test('Renders Uncontrolled ExpandableSection', () => {
const { asFragment } = render(
<ExpandableSection {...props} toggleText="Show More">
Expand All @@ -42,20 +56,6 @@ test('Renders Uncontrolled ExpandableSection', () => {
expect(asFragment()).toMatchSnapshot();
});

test('Detached ExpandableSection renders successfully', () => {
const { asFragment } = render(
<Fragment>
<ExpandableSection isExpanded isDetached {...props}>
test
</ExpandableSection>
<ExpandableSectionToggle isExpanded direction="up" {...props}>
Toggle text
</ExpandableSectionToggle>
</Fragment>
);
expect(asFragment()).toMatchSnapshot();
});

test('Disclosure ExpandableSection', () => {
const { asFragment } = render(
<ExpandableSection {...props} displaySize="lg" isWidthLimited>
Expand All @@ -75,22 +75,22 @@ test('Renders ExpandableSection indented', () => {
expect(asFragment()).toMatchSnapshot();
});

test('Does not render with pf-m-truncate class when variant is not passed', () => {
test(`Does not render with ${styles.modifiers.truncate} class when variant is not passed`, () => {
render(<ExpandableSection>test</ExpandableSection>);

expect(screen.getByText('test').parentElement).not.toHaveClass('pf-m-truncate');
expect(screen.getByText('test').parentElement).not.toHaveClass(styles.modifiers.truncate);
});

test('Does not render with pf-m-truncate class when variant is not truncate', () => {
test(`Does not render with ${styles.modifiers.truncate} class when variant is not truncate`, () => {
render(<ExpandableSection variant={ExpandableSectionVariant.default}>test</ExpandableSection>);

expect(screen.getByText('test').parentElement).not.toHaveClass('pf-m-truncate');
expect(screen.getByText('test').parentElement).not.toHaveClass(styles.modifiers.truncate);
});

test('Renders with pf-m-truncate class when variant is truncate', () => {
test(`Renders with ${styles.modifiers.truncate} class when variant is truncate`, () => {
render(<ExpandableSection variant={ExpandableSectionVariant.truncate}>test</ExpandableSection>);

expect(screen.getByText('test').parentElement).toHaveClass('pf-m-truncate');
expect(screen.getByText('test').parentElement).toHaveClass(styles.modifiers.truncate);
});

test('Renders with value passed to contentId', () => {
Expand Down Expand Up @@ -129,3 +129,65 @@ test('Renders with ARIA attributes when contentId and toggleId are passed', () =
expect(wrapper).toContainHTML('aria-labelledby="toggle-id"');
expect(wrapper).toContainHTML('aria-controls="content-id"');
});

test(`Does not render with classes ${styles.modifiers.expandTop} nor ${styles.modifiers.expandBottom} by default`, () => {
render(<ExpandableSection>Test content</ExpandableSection>);

expect(screen.getByText('Test content').parentElement).not.toHaveClass('pf-m-expand-top');
expect(screen.getByText('Test content').parentElement).not.toHaveClass('pf-m-expand-bottom');
});

test(`Does not render with classes ${styles.modifiers.expandTop} nor ${styles.modifiers.expandBottom} when only isDetached is true`, () => {
render(<ExpandableSection isDetached>Test content</ExpandableSection>);

expect(screen.getByText('Test content').parentElement).not.toHaveClass('pf-m-expand-top');
expect(screen.getByText('Test content').parentElement).not.toHaveClass('pf-m-expand-bottom');
});

test(`Does not render with class ${styles.modifiers.expandTop} when direction="up" and isDetached is false`, () => {
render(<ExpandableSection direction="up">Test content</ExpandableSection>);

expect(screen.getByText('Test content').parentElement).not.toHaveClass('pf-m-expand-top');
});

test(`Does not render with class ${styles.modifiers.expandBottom} when direction="down" and isDetached is false`, () => {
render(<ExpandableSection direction="down">Test content</ExpandableSection>);

expect(screen.getByText('Test content').parentElement).not.toHaveClass('pf-m-expand-bottom');
});

test(`Renders with class ${styles.modifiers.expandTop} when isDetached is true and direction="up"`, () => {
render(
<ExpandableSection isDetached direction="up">
Test content
</ExpandableSection>
);

expect(screen.getByText('Test content').parentElement).toHaveClass('pf-m-expand-top');
});

test(`Renders with class ${styles.modifiers.expandBottom} when isDetached is true and direction="down"`, () => {
render(
<ExpandableSection isDetached direction="down">
Test content
</ExpandableSection>
);

expect(screen.getByText('Test content').parentElement).toHaveClass('pf-m-expand-bottom');
});

test('Does not render with class pf-m-detached when isDetached is true and direction is not passed', () => {
render(<ExpandableSection isDetached>Test content</ExpandableSection>);

expect(screen.getByText('Test content').parentElement).not.toHaveClass('pf-m-detached');
});

test('Renders with class pf-m-detached when isDetached is true and direction is passed', () => {
render(
<ExpandableSection isDetached direction="up">
Test content
</ExpandableSection>
);

expect(screen.getByText('Test content').parentElement).toHaveClass('pf-m-detached');
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { ExpandableSectionToggle } from '../ExpandableSectionToggle';
import styles from '@patternfly/react-styles/css/components/ExpandableSection/expandable-section';

test('Renders without children', () => {
render(<ExpandableSectionToggle></ExpandableSectionToggle>);

expect(screen.getByRole('button')).toBeInTheDocument();
});

test('Renders with children', () => {
render(<ExpandableSectionToggle>Toggle test</ExpandableSectionToggle>);

expect(screen.getByRole('button')).toHaveTextContent('Toggle test');
});

test('Does not render with class pf-m-detached by default', () => {
render(<ExpandableSectionToggle data-testid="test-id">Toggle test</ExpandableSectionToggle>);

expect(screen.getByTestId('test-id')).not.toHaveClass('pf-m-detached');
});

test('Renders with class pf-m-detached when isDetached is true', () => {
render(
<ExpandableSectionToggle data-testid="test-id" isDetached>
Toggle test
</ExpandableSectionToggle>
);

expect(screen.getByTestId('test-id')).toHaveClass('pf-m-detached');
});
Original file line number Diff line number Diff line change
@@ -1,67 +1,5 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Detached ExpandableSection renders successfully 1`] = `
<DocumentFragment>
<div
class="pf-v6-c-expandable-section pf-m-expanded"
>
<div
aria-labelledby="toggle-id"
class="pf-v6-c-expandable-section__content"
id="content-id"
role="region"
>
test
</div>
</div>
<div
class="pf-v6-c-expandable-section pf-m-expanded"
>
<div
class="pf-v6-c-expandable-section__toggle"
>
<button
aria-controls="content-id"
aria-expanded="true"
class="pf-v6-c-button pf-m-link"
data-ouia-component-id="OUIA-Generated-Button-link-5"
data-ouia-component-type="PF6/Button"
data-ouia-safe="true"
id="toggle-id"
type="button"
>
<span
class="pf-v6-c-button__icon pf-m-start"
>
<span
class="pf-v6-c-expandable-section__toggle-icon pf-m-expand-top"
>
<svg
aria-hidden="true"
class="pf-v6-svg"
fill="currentColor"
height="1em"
role="img"
viewBox="0 0 256 512"
width="1em"
>
<path
d="M224.3 273l-136 136c-9.4 9.4-24.6 9.4-33.9 0l-22.6-22.6c-9.4-9.4-9.4-24.6 0-33.9l96.4-96.4-96.4-96.4c-9.4-9.4-9.4-24.6 0-33.9L54.3 103c9.4-9.4 24.6-9.4 33.9 0l136 136c9.5 9.4 9.5 24.6.1 34z"
/>
</svg>
</span>
</span>
<span
class="pf-v6-c-button__text"
>
Toggle text
</span>
</button>
</div>
</div>
</DocumentFragment>
`;

exports[`Disclosure ExpandableSection 1`] = `
<DocumentFragment>
<div
Expand Down Expand Up @@ -285,7 +223,7 @@ exports[`Renders Uncontrolled ExpandableSection 1`] = `
<button
aria-controls="content-id"
class="pf-v6-c-button pf-m-link"
data-ouia-component-id="OUIA-Generated-Button-link-4"
data-ouia-component-id="OUIA-Generated-Button-link-5"
data-ouia-component-type="PF6/Button"
data-ouia-safe="true"
id="toggle-id"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@kmcfaul kmcfaul May 29, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @mcoker wdyt?


```ts file="ExpandableSectionDetached.tsx"

```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const ExpandableSectionDetached: React.FunctionComponent = () => {
return (
<Stack hasGutter>
<StackItem>
<ExpandableSection isExpanded={isExpanded} isDetached toggleId={toggleId} contentId={contentId}>
<ExpandableSection isExpanded={isExpanded} isDetached direction="up" toggleId={toggleId} contentId={contentId}>
This content is visible only when the component is expanded.
</ExpandableSection>
</StackItem>
Expand Down
2 changes: 1 addition & 1 deletion packages/react-docs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"test:a11y": "patternfly-a11y --config patternfly-a11y.config"
},
"dependencies": {
"@patternfly/patternfly": "6.3.0-prerelease.29",
"@patternfly/patternfly": "6.3.0-prerelease.33",
"@patternfly/react-charts": "workspace:^",
"@patternfly/react-code-editor": "workspace:^",
"@patternfly/react-core": "workspace:^",
Expand Down
Loading
Loading