-
Notifications
You must be signed in to change notification settings - Fork 370
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
Changes from all commits
ff16d31
2e8bda6
07308f2
5fcbc65
300bfd7
a228281
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
} | ||
|
||
export const ExpandableSectionToggle: React.FunctionComponent<ExpandableSectionToggleProps> = ({ | ||
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want styles.modifiers.detached instead? Just a nit though. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
|
@@ -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 /> | ||
|
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 |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind the 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @mcoker wdyt? |
||
|
||
```ts file="ExpandableSectionDetached.tsx" | ||
|
||
``` | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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)