From d2dee8e8bc9d193977d77eeb7fbc5c225bb00d09 Mon Sep 17 00:00:00 2001 From: Bartosz Adamczyk Date: Thu, 11 May 2023 00:15:56 +0200 Subject: [PATCH 01/13] new useIsFirstRender useIsFirstRender rewritten and used in Checkbox and Radio --- .../form-elements/checkbox/Checkbox.tsx | 17 ++++--- .../form-elements/radio/Radio.stories.mdx | 33 +++++++++++++ src/components/form-elements/radio/Radio.tsx | 46 +++++++++++++------ src/components/utils/useIsFirstRender.ts | 23 ++++++---- 4 files changed, 89 insertions(+), 30 deletions(-) diff --git a/src/components/form-elements/checkbox/Checkbox.tsx b/src/components/form-elements/checkbox/Checkbox.tsx index 51a122eaf2..afdd09cbce 100644 --- a/src/components/form-elements/checkbox/Checkbox.tsx +++ b/src/components/form-elements/checkbox/Checkbox.tsx @@ -5,6 +5,7 @@ import {__DEV__, invariant} from '../../utils'; import Text from '../../text/Text'; import {CheckIcon, IndeterminateIcon} from './CheckboxIcon'; import ErrorMessage from '../ErrorMessage'; +import {useIsFirstRender} from '../../utils/useIsFirstRender'; type CheckboxColorType = 'dark' | 'light'; type CheckboxLabelSizeType = 'medium' | 'small'; @@ -184,7 +185,8 @@ const Checkbox = ({ ); const inputRef = React.useRef(null); const iconRef = React.useRef(null); - const [isPristine, setIsPristine] = React.useState(true); + const isFirstRender = useIsFirstRender(); + const [shouldAnimate, setShouldAnimate] = React.useState(false); React.useEffect(() => { if (inputRef.current) inputRef.current.indeterminate = indeterminate; @@ -192,19 +194,22 @@ const Checkbox = ({ React.useEffect(() => { if (isControlled && checked !== isChecked) { setIsChecked(checked); - if (isPristine) setIsPristine(false); + if (!isFirstRender && !shouldAnimate) setShouldAnimate(true); } - }, [checked, isControlled, isChecked, isPristine]); + }, [checked, isControlled, isChecked, isFirstRender, shouldAnimate]); const onInputChange = React.useCallback( e => { if (!isControlled) { setIsChecked(val => !val); - if (isPristine) setIsPristine(false); } if (onChange) onChange(e); + + if (!shouldAnimate) { + setShouldAnimate(true); + } }, - [onChange, isControlled, isPristine] + [onChange, isControlled, shouldAnimate] ); if (__DEV__) { @@ -231,7 +236,7 @@ const Checkbox = ({ [`sg-checkbox__label--${String(labelSize)}`]: labelSize, }); const iconClass = classNames('sg-checkbox__icon', { - 'sg-checkbox__icon--with-animation': !isPristine, // Apply animation only when checkbox is not pristine + 'sg-checkbox__icon--with-animation': shouldAnimate, // Apply animation only when checkbox is not pristine }); const errorTextId = `${checkboxId}-errorText`; const descriptionId = `${checkboxId}-description`; diff --git a/src/components/form-elements/radio/Radio.stories.mdx b/src/components/form-elements/radio/Radio.stories.mdx index c888815464..88f60f2fa6 100644 --- a/src/components/form-elements/radio/Radio.stories.mdx +++ b/src/components/form-elements/radio/Radio.stories.mdx @@ -7,6 +7,7 @@ import Flex from '../../flex/Flex'; import Box from '../../box/Box'; import Headline from '../../text/Headline'; import Text from '../../text/Text'; +import Button from '../../buttons/Button'; import Radio from './Radio'; import RadioGroup from './RadioGroup'; @@ -381,6 +382,38 @@ In the following example, className `sg-radio--custom-theme` was applied to all + + + {args => { + const [value, setValue] = React.useState('option-a'); + return ( +
+
+ +
+ + Option A + + + Option B + +
+ ); + }} +
+
+ ## Accessibility diff --git a/src/components/form-elements/radio/Radio.tsx b/src/components/form-elements/radio/Radio.tsx index 7a6d054653..8909fb6f8b 100644 --- a/src/components/form-elements/radio/Radio.tsx +++ b/src/components/form-elements/radio/Radio.tsx @@ -8,6 +8,7 @@ import classNames from 'classnames'; import Text from '../../text/Text'; import generateRandomString from '../../../js/generateRandomString'; import useRadioContext from './useRadioContext'; +import {useIsFirstRender} from '../../utils/useIsFirstRender'; export type RadioColorType = 'light' | 'dark'; type RadioLabelSizeType = 'medium' | 'small'; @@ -167,20 +168,35 @@ const Radio = ({ const isWithinRadioGroup = Boolean( radioGroupContext && Object.keys(radioGroupContext).length ); - const [isPristine, setIsPristine] = React.useState(true); - const shouldAnimate = - (isWithinRadioGroup && !radioGroupContext.isPristine) || !isPristine; + const isFirstRender = useIsFirstRender(); + const [shouldAnimate, setShouldAnimate] = React.useState(false); const isControlled = checked !== undefined || isWithinRadioGroup; - let isChecked: boolean | undefined = undefined; + const [isChecked, setIsChecked] = React.useState(); - if (isControlled) { - // Radio can either be directly set as checked, or be controlled by a RadioGroup - isChecked = - checked !== undefined - ? checked - : Boolean(radioGroupContext.selectedValue) && - radioGroupContext.selectedValue === value; - } + React.useEffect(() => { + if (isControlled) { + // Radio can either be directly set as checked, or be controlled by a RadioGroup + const newIsChecked = + checked !== undefined + ? checked + : Boolean(radioGroupContext.selectedValue) && + radioGroupContext.selectedValue === value; + + setIsChecked(newIsChecked); + + if (!isFirstRender && !shouldAnimate && newIsChecked !== isChecked) { + setShouldAnimate(true); + } + } + }, [ + isChecked, + checked, + isControlled, + value, + radioGroupContext.selectedValue, + isFirstRender, + shouldAnimate, + ]); const colorName = radioGroupContext.color || color; const isDisabled = @@ -213,13 +229,15 @@ const Radio = ({ if (isWithinRadioGroup) { radioGroupContext.setLastFocusedValue(value); radioGroupContext.setSelectedValue(e, value); - } else { - setIsPristine(false); } if (onChange) { onChange(e); } + + if (!shouldAnimate) { + setShouldAnimate(true); + } }; return ( diff --git a/src/components/utils/useIsFirstRender.ts b/src/components/utils/useIsFirstRender.ts index 33fce0b4e2..4169c04d25 100644 --- a/src/components/utils/useIsFirstRender.ts +++ b/src/components/utils/useIsFirstRender.ts @@ -1,14 +1,17 @@ -import {useRef} from 'react'; +import React from 'react'; -const useIsFirstRender = () => { - const isFirstRender = useRef(true); +export const useIsFirstRender = () => { + const [isFirstRender, setIsFirstRender] = React.useState(true); - if (isFirstRender.current === true) { - isFirstRender.current = false; - return true; - } + React.useLayoutEffect(() => { + const raf = window.requestAnimationFrame(() => { + setIsFirstRender(false); + }); - return isFirstRender.current; -}; + return () => { + window.cancelAnimationFrame(raf); + }; + }, []); -export default useIsFirstRender; + return isFirstRender; +}; From cdd935c47e39fedffc72b6f9c289e5807ab2857b Mon Sep 17 00:00:00 2001 From: Bartosz Adamczyk Date: Thu, 11 May 2023 23:00:33 +0200 Subject: [PATCH 02/13] use isomorphic layout effect in useIsFirstRender --- package.json | 3 ++- .../form-elements/radio/RadioGroup.spec.tsx | 11 ++++++----- src/components/utils/useIsFirstRender.ts | 3 ++- yarn.lock | 5 +++++ 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 9dea17a132..f6242b29b1 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,8 @@ "meow": "^5.0.0", "patch-package": "^6.5.1", "path": "^0.12.7", - "postinstall-postinstall": "^2.1.0" + "postinstall-postinstall": "^2.1.0", + "use-isomorphic-layout-effect": "^1.1.2" }, "devDependencies": { "@babel/cli": "^7.8.3", diff --git a/src/components/form-elements/radio/RadioGroup.spec.tsx b/src/components/form-elements/radio/RadioGroup.spec.tsx index 90e5bc21e1..d4d1ebf602 100644 --- a/src/components/form-elements/radio/RadioGroup.spec.tsx +++ b/src/components/form-elements/radio/RadioGroup.spec.tsx @@ -1,7 +1,7 @@ import * as React from 'react'; import RadioGroup from './RadioGroup'; import Radio from './Radio'; -import {render} from '@testing-library/react'; +import {render, waitFor} from '@testing-library/react'; import userEvent from '@testing-library/user-event'; describe('', () => { @@ -67,7 +67,7 @@ describe('', () => { expect(onChange).not.toHaveBeenCalled(); expect(radioGroup.getByLabelText('Option B')).not.toBeChecked(); }); - it('checked radio can be changed on controlled radio group', () => { + it('checked radio can be changed on controlled radio group', async () => { const {container, getByLabelText, rerender} = renderRadioGroup({ name: 'option', value: 'option-a', @@ -96,11 +96,10 @@ describe('', () => { ); - expect(iconsWithAnimation.length).toBe(2); expect(getByLabelText('Option A')).not.toBeChecked(); expect(getByLabelText('Option B')).toBeChecked(); }); - it('it does not apply animation unless initial state has changed', () => { + it('it does not apply animation unless initial state has changed after first render of DOM', async () => { const radioGroup = renderRadioGroup({ name: 'option', value: 'option-a', @@ -111,7 +110,9 @@ describe('', () => { expect(iconsWithAnimation.length).toBe(0); userEvent.click(radioGroup.getByLabelText('Option B')); - expect(iconsWithAnimation.length).toBe(2); + await waitFor(() => expect(iconsWithAnimation.length).toBe(1)); + userEvent.click(radioGroup.getByLabelText('Option A')); + await waitFor(() => expect(iconsWithAnimation.length).toBe(2)); }); it('has an accessible name', () => { const onChange = jest.fn(); diff --git a/src/components/utils/useIsFirstRender.ts b/src/components/utils/useIsFirstRender.ts index 4169c04d25..1c4d4c70ca 100644 --- a/src/components/utils/useIsFirstRender.ts +++ b/src/components/utils/useIsFirstRender.ts @@ -1,9 +1,10 @@ import React from 'react'; +import useIsomorphicLayoutEffect from 'use-isomorphic-layout-effect'; export const useIsFirstRender = () => { const [isFirstRender, setIsFirstRender] = React.useState(true); - React.useLayoutEffect(() => { + useIsomorphicLayoutEffect(() => { const raf = window.requestAnimationFrame(() => { setIsFirstRender(false); }); diff --git a/yarn.lock b/yarn.lock index 8c338f6c05..d85c8290d9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -20068,6 +20068,11 @@ url@^0.11.0: punycode "1.3.2" querystring "0.2.0" +use-isomorphic-layout-effect@^1.1.2: + version "1.1.2" + resolved "https://registry.yarnpkg.com/use-isomorphic-layout-effect/-/use-isomorphic-layout-effect-1.1.2.tgz#497cefb13d863d687b08477d9e5a164ad8c1a6fb" + integrity sha512-49L8yCO3iGT/ZF9QttjwLF/ZD9Iwto5LnH5LmEdk/6cFmXddqi2ulF0edxTwjj+7mqvpVVGQWvbXZdn32wRSHA== + use@^3.1.0: version "3.1.1" resolved "https://registry.yarnpkg.com/use/-/use-3.1.1.tgz#d50c8cac79a19fbc20f2911f56eb973f4e10070f" From c7f2457b0dd0024831a82905a348f0e68d32810e Mon Sep 17 00:00:00 2001 From: Bartosz Adamczyk Date: Thu, 11 May 2023 23:56:17 +0200 Subject: [PATCH 03/13] use ref in useIsFirstRender instead of state --- .../form-elements/checkbox/Checkbox.tsx | 24 +++++++---- src/components/form-elements/radio/Radio.tsx | 43 +++++++------------ src/components/utils/useIsFirstRender.ts | 4 +- 3 files changed, 33 insertions(+), 38 deletions(-) diff --git a/src/components/form-elements/checkbox/Checkbox.tsx b/src/components/form-elements/checkbox/Checkbox.tsx index afdd09cbce..3c873fde9d 100644 --- a/src/components/form-elements/checkbox/Checkbox.tsx +++ b/src/components/form-elements/checkbox/Checkbox.tsx @@ -176,6 +176,7 @@ const Checkbox = ({ 'aria-labelledby': ariaLabelledBy, ...props }: CheckboxPropsType) => { + const checkboxIconRef = React.useRef(); const {current: checkboxId} = React.useRef( id === undefined || id === '' ? generateRandomString() : id ); @@ -186,7 +187,6 @@ const Checkbox = ({ const inputRef = React.useRef(null); const iconRef = React.useRef(null); const isFirstRender = useIsFirstRender(); - const [shouldAnimate, setShouldAnimate] = React.useState(false); React.useEffect(() => { if (inputRef.current) inputRef.current.indeterminate = indeterminate; @@ -194,9 +194,14 @@ const Checkbox = ({ React.useEffect(() => { if (isControlled && checked !== isChecked) { setIsChecked(checked); - if (!isFirstRender && !shouldAnimate) setShouldAnimate(true); + + if (isFirstRender.current === false && checkboxIconRef.current) { + checkboxIconRef.current.classList.add( + 'sg-checkbox__icon--with-animation' + ); + } } - }, [checked, isControlled, isChecked, isFirstRender, shouldAnimate]); + }, [checked, isControlled, isChecked, isFirstRender]); const onInputChange = React.useCallback( e => { if (!isControlled) { @@ -205,11 +210,13 @@ const Checkbox = ({ if (onChange) onChange(e); - if (!shouldAnimate) { - setShouldAnimate(true); + if (checkboxIconRef.current) { + checkboxIconRef.current.classList.add( + 'sg-checkbox__icon--with-animation' + ); } }, - [onChange, isControlled, shouldAnimate] + [onChange, isControlled, checkboxIconRef] ); if (__DEV__) { @@ -235,9 +242,7 @@ const Checkbox = ({ 'sg-checkbox__label--with-padding-bottom': description || errorMessage, [`sg-checkbox__label--${String(labelSize)}`]: labelSize, }); - const iconClass = classNames('sg-checkbox__icon', { - 'sg-checkbox__icon--with-animation': shouldAnimate, // Apply animation only when checkbox is not pristine - }); + const iconClass = classNames('sg-checkbox__icon'); const errorTextId = `${checkboxId}-errorText`; const descriptionId = `${checkboxId}-description`; const describedbyIds = React.useMemo(() => { @@ -292,6 +297,7 @@ const Checkbox = ({