From ea47fa2068730c6bfac33c1cf7852f22d9368f93 Mon Sep 17 00:00:00 2001 From: Alex Page Date: Fri, 13 Nov 2020 14:50:36 -0500 Subject: [PATCH 1/3] functional component and no jank when opening --- UNRELEASED.md | 1 + scripts/build-validate.js | 9 - src/components/Collapsible/Collapsible.scss | 25 +- src/components/Collapsible/Collapsible.tsx | 215 +++++------------- src/components/Collapsible/README.md | 73 +++++- .../Collapsible/tests/Collapsible.test.tsx | 16 -- 6 files changed, 138 insertions(+), 201 deletions(-) diff --git a/UNRELEASED.md b/UNRELEASED.md index 94ed837f742..f1efe377f01 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -18,6 +18,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f - Removed extra bottom border on the `DataTable` and added curved edges to footers ([#3571](https://github.com/Shopify/polaris-react/pull/3571)) - **`Button`:** `loading` no longer sets the invalid `role="alert"` ([#3590](https://github.com/Shopify/polaris-react/pull/3590)) - Removed `tabIndex=-1` from `Popover` when `preventAutoFocus` is true ([#3595](https://github.com/Shopify/polaris-react/pull/3595)) +- Fix `Filters` janky animation when opening and closing ([#3606](https://github.com/Shopify/polaris-react/pull/3606)) - Fixed `Modal` header border color ([#3616](https://github.com/Shopify/polaris-react/pull/3616)) - Fixed `TopBar` search clear button alignment on iOS ([#3618](https://github.com/Shopify/polaris-react/pull/3618)) diff --git a/scripts/build-validate.js b/scripts/build-validate.js index 0347f61b370..181b5d9434d 100644 --- a/scripts/build-validate.js +++ b/scripts/build-validate.js @@ -58,15 +58,6 @@ function validateEsNextBuild() { assert.ok(jsContent.includes("import './Avatar.css';")); assert.ok(jsContent.includes('"Avatar": "Polaris-Avatar_z763p"')); assert.ok(jsContent.includes('"hidden": "Polaris-Avatar--hidden_riqie"')); - - assert.ok( - fs - .readFileSync( - './dist/esnext/components/Collapsible/Collapsible.tsx.esnext', - 'utf-8', - ) - .includes('class Collapsible'), - ); } function validateSassPublicApi() { diff --git a/src/components/Collapsible/Collapsible.scss b/src/components/Collapsible/Collapsible.scss index 4513cc64fef..2f7f2fc7b2c 100644 --- a/src/components/Collapsible/Collapsible.scss +++ b/src/components/Collapsible/Collapsible.scss @@ -1,33 +1,30 @@ @import '../../styles/common'; .Collapsible { - overflow: hidden; - max-height: 0; padding-top: 0; padding-bottom: 0; - opacity: 0; - will-change: opacity, max-height; -} - -.animating { - transition-property: opacity, max-height; - transition-duration: duration(slow); + height: 0; + overflow: hidden; + will-change: height; + transition-property: height; + transition-duration: duration(fast); transition-timing-function: easing(out); } .open { - opacity: 1; + height: auto; + overflow: visible; } -.fullyOpen { - overflow: visible; +// Stop children from being focused when aria-hidden +.Collapsible[aria-hidden='true'] { + display: none; } .expandOnPrint { @include when-printing { - opacity: 1; // stylelint-disable-next-line declaration-no-important - max-height: none !important; + height: auto !important; overflow: visible; } } diff --git a/src/components/Collapsible/Collapsible.tsx b/src/components/Collapsible/Collapsible.tsx index 73e59633acf..b7551c181a0 100644 --- a/src/components/Collapsible/Collapsible.tsx +++ b/src/components/Collapsible/Collapsible.tsx @@ -1,10 +1,4 @@ -import React, { - createContext, - createRef, - TransitionEvent, - Component, - ComponentClass, -} from 'react'; +import React, {useState, useRef, useEffect} from 'react'; import {classNames} from '../../utilities/css'; @@ -30,165 +24,70 @@ export interface CollapsibleProps { children?: React.ReactNode; } -type AnimationState = - | 'idle' - | 'measuring' - | 'closingStart' - | 'closing' - | 'openingStart' - | 'opening'; - -interface State { - height?: number | null; - animationState: AnimationState; - open: boolean; -} - -const ParentCollapsibleExpandingContext = createContext(false); - -class CollapsibleInner extends Component { - static contextType = ParentCollapsibleExpandingContext; - - static getDerivedStateFromProps( - {open: willOpen}: CollapsibleProps, - {open, animationState: prevAnimationState}: State, - ) { - let nextAnimationState = prevAnimationState; - if (open !== willOpen) { - nextAnimationState = 'measuring'; - } - - return { - animationState: nextAnimationState, - open: willOpen, - }; - } - - context!: React.ContextType; - - state: State = { - height: null, - animationState: 'idle', - // eslint-disable-next-line react/no-unused-state - open: this.props.open, +export function Collapsible({ + id, + expandOnPrint, + open, + transition, + children, +}: CollapsibleProps) { + const [height, setHeight] = useState(null); + const [isOpen, setIsOpen] = useState(open); + const collapisbleContainer = useRef(null); + + const wrapperClassName = classNames( + styles.Collapsible, + expandOnPrint && styles.expandOnPrint, + isOpen && styles.open, + height && styles.animating, + ); + + const collapsibleStyles = { + ...(transition && { + transitionDuration: `${transition.duration}`, + transitionTimingFunction: `${transition.timingFunction}`, + }), + ...(typeof height === 'number' && { + height: `${height}px`, + overflow: 'hidden', + }), }; - private node = createRef(); - private heightNode = createRef(); - - componentDidUpdate({open: wasOpen}: CollapsibleProps) { - const {animationState} = this.state; - const parentCollapsibleExpanding = this.context; - - if (parentCollapsibleExpanding && animationState !== 'idle') { - // eslint-disable-next-line react/no-did-update-set-state - this.setState({ - animationState: 'idle', - }); - + // Measure the child height for open and close + useEffect(() => { + if (open === isOpen || !collapisbleContainer.current) { return; } - requestAnimationFrame(() => { - const heightNode = this.heightNode.current; - switch (animationState) { - case 'idle': - break; - case 'measuring': - this.setState({ - animationState: wasOpen ? 'closingStart' : 'openingStart', - height: wasOpen && heightNode ? heightNode.scrollHeight : 0, - }); - break; - case 'closingStart': - this.setState({ - animationState: 'closing', - height: 0, - }); - break; - case 'openingStart': - this.setState({ - animationState: 'opening', - height: heightNode ? heightNode.scrollHeight : 0, - }); - } - }); - } + setHeight(collapisbleContainer.current.scrollHeight); + }, [open, isOpen]); - render() { - const {id, expandOnPrint, open, children, transition} = this.props; - const {animationState, height} = this.state; - const parentCollapsibleExpanding = this.context; - - const animating = animationState !== 'idle'; - - const wrapperClassName = classNames( - styles.Collapsible, - open && styles.open, - animating && styles.animating, - !animating && open && styles.fullyOpen, - expandOnPrint && styles.expandOnPrint, - ); - - const displayHeight = collapsibleHeight(open, animationState, height); - - const content = animating || open || expandOnPrint ? children : null; - - const transitionProperties = transition - ? { - transitionDuration: `${transition.duration}`, - transitionTimingFunction: `${transition.timingFunction}`, - } - : null; - - return ( - -
-
{content}
-
- - ); - } - - private handleTransitionEnd = (event: TransitionEvent) => { - const {target} = event; - if (target === this.node.current) { - this.setState({animationState: 'idle', height: null}); + // If closing, set the height zero on the next render + useEffect(() => { + if (open || height === null || !collapisbleContainer.current) { + return; } - }; -} -function collapsibleHeight( - open: boolean, - animationState: AnimationState, - height?: number | null, -) { - if (animationState === 'idle' && open) { - return open ? 'none' : undefined; - } + getComputedStyle(collapisbleContainer.current).height; + setHeight(0); + }, [height, open]); - if (animationState === 'measuring') { - return open ? undefined : 'none'; - } + // When animation is complete clean up + const handleCompleteAnimation = () => { + setHeight(null); + setIsOpen(open); + }; - return `${height || 0}px`; + return ( +
handleCompleteAnimation()} + ref={collapisbleContainer} + aria-hidden={!open && !isOpen} + > + {children} +
+ ); } - -export const Collapsible = CollapsibleInner as ComponentClass< - CollapsibleProps -> & - typeof CollapsibleInner; diff --git a/src/components/Collapsible/README.md b/src/components/Collapsible/README.md index 00317ebc1a8..cbc149737bb 100644 --- a/src/components/Collapsible/README.md +++ b/src/components/Collapsible/README.md @@ -57,9 +57,9 @@ Use for a basic “show more” interaction when you need to display more conten ```jsx function CollapsibleExample() { - const [active, setActive] = useState(true); + const [open, setOpen] = useState(false); - const handleToggle = useCallback(() => setActive((active) => !active), []); + const handleToggle = useCallback(() => setOpen((open) => !open), []); return (
@@ -67,15 +67,16 @@ function CollapsibleExample() { Your mailing list lets you contact customers or visitors who have @@ -90,6 +91,70 @@ function CollapsibleExample() { } ``` +### Nested collapsible + +When you have multiple collapsibles inside each other. This should be avoided as it causes the user to open multiple collapsible areas. + +```jsx +function CollapsibleExample() { + const [open, setOpen] = useState(true); + const [innerOpen, setInnerOpen] = useState(false); + + const handleToggle = useCallback(() => setOpen((open) => !open), []); + const handleInnerToggle = useCallback( + () => setInnerOpen((open) => !open), + [], + ); + + return ( +
+ + + + + + Your mailing list lets you contact customers or visitors who have + shown an interest in your store. Reach out to them with exclusive + offers or updates about your products. + + + + + Your mailing list lets you contact customers or visitors who + have shown an interest in your store. Reach out to them with + exclusive offers or updates about your products. + + + + + +
+ ); +} +``` + ![Collapsible on Android](/public_images/components/Collapsible/android/default@2x.png) diff --git a/src/components/Collapsible/tests/Collapsible.test.tsx b/src/components/Collapsible/tests/Collapsible.test.tsx index 0f6dfa436c4..16700294d6c 100644 --- a/src/components/Collapsible/tests/Collapsible.test.tsx +++ b/src/components/Collapsible/tests/Collapsible.test.tsx @@ -17,22 +17,6 @@ describe('', () => { const hidden = collapsible.find(ariaHiddenSelector); expect(hidden.exists()).toBe(true); - expect(collapsible.contains('content')).toBe(false); - }); - - it('does not render its children when going from open to closed', () => { - const Child = () => null; - - const collapsible = mountWithAppProvider( - - - , - ); - - expect(collapsible.find(Child)).toHaveLength(1); - collapsible.setProps({open: false}); - collapsible.simulate('transitionEnd'); - expect(collapsible.find(Child)).toHaveLength(0); }); it('renders its children and does not render aria-hidden when open', () => { From 90118585df5d659cef1c5b787ed121646bd3d17f Mon Sep 17 00:00:00 2001 From: Alex Page Date: Mon, 7 Dec 2020 07:57:45 -0800 Subject: [PATCH 2/3] Update src/components/Collapsible/README.md Co-authored-by: Andrew Musgrave --- src/components/Collapsible/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Collapsible/README.md b/src/components/Collapsible/README.md index cbc149737bb..cb88f39647e 100644 --- a/src/components/Collapsible/README.md +++ b/src/components/Collapsible/README.md @@ -96,7 +96,7 @@ function CollapsibleExample() { When you have multiple collapsibles inside each other. This should be avoided as it causes the user to open multiple collapsible areas. ```jsx -function CollapsibleExample() { +function NestedCollapsibleExample() { const [open, setOpen] = useState(true); const [innerOpen, setInnerOpen] = useState(false); From 4e0c287f6f071e093335fe84de6720f2b73e2772 Mon Sep 17 00:00:00 2001 From: Alex Page Date: Thu, 17 Dec 2020 08:02:56 -0800 Subject: [PATCH 3/3] Working collapsible, needs display none when closed for a11y --- src/components/Collapsible/Collapsible.scss | 6 +++--- src/components/Collapsible/Collapsible.tsx | 20 +++++++++++++------- src/components/Collapsible/README.md | 2 +- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/components/Collapsible/Collapsible.scss b/src/components/Collapsible/Collapsible.scss index 2f7f2fc7b2c..7ea844051dc 100644 --- a/src/components/Collapsible/Collapsible.scss +++ b/src/components/Collapsible/Collapsible.scss @@ -17,9 +17,9 @@ } // Stop children from being focused when aria-hidden -.Collapsible[aria-hidden='true'] { - display: none; -} +// .Collapsible[aria-hidden='true'] { +// display: none; +// } .expandOnPrint { @include when-printing { diff --git a/src/components/Collapsible/Collapsible.tsx b/src/components/Collapsible/Collapsible.tsx index b7551c181a0..d7faa08f41a 100644 --- a/src/components/Collapsible/Collapsible.tsx +++ b/src/components/Collapsible/Collapsible.tsx @@ -53,6 +53,12 @@ export function Collapsible({ }), }; + // When animation is complete clean up + const handleCompleteAnimation = () => { + setHeight(null); + setIsOpen(open); + }; + // Measure the child height for open and close useEffect(() => { if (open === isOpen || !collapisbleContainer.current) { @@ -68,16 +74,16 @@ export function Collapsible({ return; } + // If it is currently animating put it back to zero + if (height !== collapisbleContainer.current.scrollHeight) { + setHeight(0); + return; + } + getComputedStyle(collapisbleContainer.current).height; setHeight(0); }, [height, open]); - // When animation is complete clean up - const handleCompleteAnimation = () => { - setHeight(null); - setIsOpen(open); - }; - return (
handleCompleteAnimation()} ref={collapisbleContainer} - aria-hidden={!open && !isOpen} + // aria-hidden={!open && !isOpen} > {children}
diff --git a/src/components/Collapsible/README.md b/src/components/Collapsible/README.md index cb88f39647e..ad9b4f0bf54 100644 --- a/src/components/Collapsible/README.md +++ b/src/components/Collapsible/README.md @@ -75,7 +75,7 @@ function CollapsibleExample() {