Skip to content

Commit 81fd3fd

Browse files
[Filters][Collapsible] Add focus on filter when collapsible animation ends (#7915)
### WHY are these changes introduced? Fixes #7822 In the Filters, opening a collapsible activates the Focus immediately. In cases like having a combobox within the collapsible, this causes a visual bug where the combobox is focused and opened prematurely. The combobox continues its transition animation, while the popover stays in place. ### WHAT is this pull request doing? #### Summary of the changes committed - Introduce the `onAnimationEnd` prop to `<Collapsible />`, a callback that gets called after the collapsible animation occurs - Set `readyForFocus` state when toggling a collapsible in `<Filters />` - Set `readyForFocus` to false on `toggleFilter` - Set `readyForFocus` to true on Collapsible's `onAnimationEnd` <details> <summary>Before</summary> Click on 'More filters' and open the 'Tagged with' collapsible. The combobox popover partially covers the textfield. <img src="https://user-images.githubusercontent.com/104938709/207687968-6df6b1d6-dde0-4f36-9097-b3aabfdf182c.gif" alt="Click on 'More filters' and open the 'Tagged with' collapsible. The combobox popover partially covers the textfield." /> </details> <details> <summary>After</summary> Click on 'More filters' and open the 'Tagged with' collapsible. The combobox popover appears beneath the textfield. <img src="https://user-images.githubusercontent.com/104938709/207687984-b7747c5d-ae60-4ab0-bd69-8c9501510afc.gif" alt="Click on 'More filters' and open the 'Tagged with' collapsible. The combobox popover partially covers the textfield."> </details>
1 parent e7712e7 commit 81fd3fd

File tree

5 files changed

+80
-12
lines changed

5 files changed

+80
-12
lines changed

.changeset/shiny-fans-build.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@shopify/polaris': minor
3+
---
4+
5+
- Added the `onAnimationEnd` prop to `Collapsible`
6+
- Fixed a bug in `Filters` where focus was moved to collapsed filter contents before the `Collapsible` animation ended

polaris-react/src/components/Collapsible/Collapsible.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ export interface CollapsibleProps {
2424
transition?: boolean | Transition;
2525
/** @deprecated Re-measuring is no longer necessary on children update **/
2626
preventMeasuringOnChildrenUpdate?: boolean;
27+
/** Callback when the animation completes. */
28+
onAnimationEnd?(): void;
2729
/** The content to display inside the collapsible. */
2830
children?: React.ReactNode;
2931
}
@@ -37,6 +39,7 @@ export function Collapsible({
3739
transition = true,
3840
preventMeasuringOnChildrenUpdate: _preventMeasuringOnChildrenUpdate,
3941
children,
42+
onAnimationEnd,
4043
}: CollapsibleProps) {
4144
const [height, setHeight] = useState(0);
4245
const [isOpen, setIsOpen] = useState(open);
@@ -73,9 +76,10 @@ export function Collapsible({
7376
if (target === collapsibleContainer.current) {
7477
setAnimationState('idle');
7578
setIsOpen(open);
79+
onAnimationEnd && onAnimationEnd();
7680
}
7781
},
78-
[open],
82+
[onAnimationEnd, open],
7983
);
8084

8185
const startAnimation = useCallback(() => {

polaris-react/src/components/Collapsible/tests/Collapsible.test.tsx

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,21 @@ describe('<Collapsible />', () => {
232232
});
233233
});
234234

235+
it('calls onAnimationEnd when provided', () => {
236+
const onAnimationEnd = jest.fn();
237+
const collapsible = mountWithApp<CollapsibleProps>(
238+
<Collapsible id="test-collapsible" open onAnimationEnd={onAnimationEnd}>
239+
content
240+
</Collapsible>,
241+
);
242+
243+
collapsible.setProps({open: false});
244+
245+
fireTransitionEnd(collapsible);
246+
247+
expect(onAnimationEnd).toHaveBeenCalledTimes(1);
248+
});
249+
235250
it('does not complete opening transition if onTransitionEnd fires on a different target', () => {
236251
const id = 'test-collapsible';
237252
const collapsible = mountWithApp<CollapsibleProps>(

polaris-react/src/components/Filters/Filters.tsx

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,11 @@ class FiltersInner extends Component<CombinedProps, State> {
208208
</div>
209209
{appliedFilterBadgeMarkup}
210210
</button>
211-
<Collapsible id={collapsibleID} open={filterIsOpen}>
211+
<Collapsible
212+
id={collapsibleID}
213+
open={filterIsOpen}
214+
onAnimationEnd={this.setReadyForFocus(true)}
215+
>
212216
<div className={styles.FilterNodeContainer}>
213217
<Focus
214218
disabled={!filterIsOpen || !readyForFocus || !open}
@@ -502,19 +506,11 @@ class FiltersInner extends Component<CombinedProps, State> {
502506
this.setState({readyForFocus: newState});
503507
};
504508

505-
private openFilter(key: string) {
506-
this.setState({[`${key}${Suffix.Filter}`]: true});
507-
}
508-
509-
private closeFilter(key: string) {
510-
this.setState({[`${key}${Suffix.Filter}`]: false});
511-
}
512-
513509
private toggleFilter(key: string) {
514510
if (this.state[`${key}${Suffix.Filter}`] === true) {
515-
this.closeFilter(key);
511+
this.setState({readyForFocus: false, [`${key}${Suffix.Filter}`]: false});
516512
} else {
517-
this.openFilter(key);
513+
this.setState({readyForFocus: false, [`${key}${Suffix.Filter}`]: true});
518514
}
519515
}
520516

polaris-react/src/components/Filters/tests/Filters.test.tsx

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {Filters, FiltersProps} from '../Filters';
1515
import {ConnectedFilterControl, TagsWrapper} from '../components';
1616
import * as focusUtils from '../../../utilities/focus';
1717
import styles from '../Filters.scss';
18+
import {Focus} from '../../Focus';
1819

1920
const MockFilter = (props: {id: string}) => <div id={props.id} />;
2021
const MockChild = () => <div />;
@@ -725,6 +726,52 @@ describe('<Filters />', () => {
725726
});
726727
});
727728
});
729+
730+
describe('readyForFocus', () => {
731+
it('unfocuses the filter when a filter is toggled', () => {
732+
const resourceFilters = mountWithApp(<Filters {...mockProps} />);
733+
734+
resourceFilters
735+
.find(Button, {children: 'More filters'})!
736+
.trigger('onClick');
737+
738+
resourceFilters
739+
.find('button', {id: 'filterOneToggleButton'})!
740+
.trigger('onClick');
741+
742+
expect(
743+
resourceFilters.find(Collapsible, {id: 'filterOneCollapsible'})!,
744+
).toContainReactComponent(Focus, {
745+
disabled: true,
746+
});
747+
});
748+
749+
it('focuses the filter when Collapsible is opened', () => {
750+
const resourceFilters = mountWithApp(<Filters {...mockProps} />);
751+
752+
resourceFilters
753+
.find(Button, {children: 'More filters'})!
754+
.trigger('onClick');
755+
756+
resourceFilters
757+
.find('button', {id: 'filterOneToggleButton'})!
758+
.trigger('onClick');
759+
760+
expect(
761+
resourceFilters.find(Collapsible, {id: 'filterOneCollapsible'})!,
762+
).toContainReactComponent(Focus, {
763+
disabled: true,
764+
});
765+
766+
resourceFilters.find(Collapsible)!.trigger('onAnimationEnd');
767+
768+
expect(
769+
resourceFilters.find(Collapsible, {id: 'filterOneCollapsible'})!,
770+
).toContainReactComponent(Focus, {
771+
disabled: false,
772+
});
773+
});
774+
});
728775
});
729776

730777
function noop() {}

0 commit comments

Comments
 (0)