Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Collapsible] Fix collapsible transition behaviour #4000

Merged
merged 1 commit into from
Feb 18, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f
- Fixed accessibility issue on `Tabs` disclosure popover on close ([#3994](https://github.com/Shopify/polaris-react/pull/3994))
- Fixed accessibility issue when tabbing into `IndexTable` ([#4004](https://github.com/Shopify/polaris-react/pull/4004))
- Fixed an issue where inline code would be hard to select ([#4005](https://github.com/Shopify/polaris-react/pull/4005))
- Fixed `Collapsible` bug where animation complete logic was being prematurely triggered by transitions in the children ([#4000](https://github.com/Shopify/polaris-react/pull/4000))

### Documentation

Expand Down
27 changes: 16 additions & 11 deletions src/components/Collapsible/Collapsible.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function Collapsible({
const [height, setHeight] = useState(0);
const [isOpen, setIsOpen] = useState(open);
const [animationState, setAnimationState] = useState<AnimationState>('idle');
const collapisbleContainer = useRef<HTMLDivElement>(null);
const collapsibleContainer = useRef<HTMLDivElement>(null);

const isFullyOpen = animationState === 'idle' && open && isOpen;
const isFullyClosed = animationState === 'idle' && !open && !isOpen;
Expand All @@ -59,10 +59,15 @@ export function Collapsible({
},
};

const handleCompleteAnimation = useCallback(() => {
setAnimationState('idle');
setIsOpen(open);
}, [open]);
const handleCompleteAnimation = useCallback(
({target}: React.TransitionEvent<HTMLDivElement>) => {
if (target === collapsibleContainer.current) {
setAnimationState('idle');
setIsOpen(open);
}
},
[open],
);

useEffect(() => {
if (open !== isOpen) {
Expand All @@ -71,32 +76,32 @@ export function Collapsible({
}, [open, isOpen]);

useEffect(() => {
if (!open || !collapisbleContainer.current) return;
if (!open || !collapsibleContainer.current) return;
// If collapsible defaults to open, set an initial height
setHeight(collapisbleContainer.current.scrollHeight);
setHeight(collapsibleContainer.current.scrollHeight);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
if (!collapisbleContainer.current) return;
if (!collapsibleContainer.current) return;

switch (animationState) {
case 'idle':
break;
case 'measuring':
setHeight(collapisbleContainer.current.scrollHeight);
setHeight(collapsibleContainer.current.scrollHeight);
setAnimationState('animating');
break;
case 'animating':
setHeight(open ? collapisbleContainer.current.scrollHeight : 0);
setHeight(open ? collapsibleContainer.current.scrollHeight : 0);
}
}, [animationState, open, isOpen]);

return (
<div
id={id}
style={collapsibleStyles}
ref={collapisbleContainer}
ref={collapsibleContainer}
className={wrapperClassName}
onTransitionEnd={handleCompleteAnimation}
aria-expanded={open}
Expand Down
60 changes: 58 additions & 2 deletions src/components/Collapsible/tests/Collapsible.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import React from 'react';
import React, {useState, useCallback} from 'react';
// eslint-disable-next-line no-restricted-imports
import {mountWithAppProvider} from 'test-utilities/legacy';
import {mountWithApp} from 'test-utilities/react-testing';
import {Tokens} from 'utilities/theme';

import {Collapsible} from '../Collapsible';
import {Collapsible, CollapsibleProps} from '../Collapsible';

describe('<Collapsible />', () => {
const ariaExpandedSelector = '[aria-expanded=false]';
Expand Down Expand Up @@ -84,4 +85,59 @@ describe('<Collapsible />', () => {
expect(collapsible.props()).toMatchObject({transition: {timingFunction}});
});
});

describe('onTransitionEnd', () => {
it('adds an isFullyClosed class to the collapsible onTransitionEnd if the event target is the collapsible div', () => {
const id = 'test-collapsible';
const collapsibleWithToggle = mountWithApp(
<CollapsibleWithToggle id={id} />,
);
collapsibleWithToggle.find('button')!.trigger('onClick');

const wrapper = collapsibleWithToggle.find('div', {id})!;
wrapper.trigger('onTransitionEnd', {
target: wrapper.domNode as HTMLDivElement,
});

expect(
collapsibleWithToggle.find('div', {
id,
'aria-expanded': false,
className: 'Collapsible isFullyClosed',
}),
).not.toBeNull();
});

it('does not add an isFullyClosed class to the collapsible onTransitionEnd if the event target is not the collapsible div', () => {
const id = 'test-collapsible';
const collapsibleWithToggle = mountWithApp(
<CollapsibleWithToggle id={id} />,
);
collapsibleWithToggle.find('button')!.trigger('onClick');

collapsibleWithToggle.find('div', {id})!.trigger('onTransitionEnd', {
target: document.createElement('div'),
});

expect(
collapsibleWithToggle.find('div', {
id,
'aria-expanded': false,
className: 'Collapsible',
}),
).not.toBeNull();
});
});
});

function CollapsibleWithToggle(props: Omit<CollapsibleProps, 'open'>) {
const [open, setOpen] = useState(true);
const handleToggle = useCallback(() => setOpen((open) => !open), []);

return (
<>
<button onClick={handleToggle}>Activator</button>
<Collapsible {...props} open={open} />
</>
);
}