From 35ba8af46c6e063c46c0680511e0d534a7d2857a Mon Sep 17 00:00:00 2001 From: Xiaofan Wu Date: Tue, 18 Oct 2022 16:48:14 +1100 Subject: [PATCH] fix: checkboxgroup all when children are updated --- .../CheckboxGroup/CheckboxGroupContext.jsx | 30 +++------ src/components/CheckboxGroup/index.d.ts | 4 ++ src/components/CheckboxGroup/index.jsx | 16 ++--- src/components/CheckboxGroup/index.spec.jsx | 61 +++++++++++++++++-- www/containers/props.json | 9 ++- www/examples/CheckboxGroup.mdx | 8 +-- 6 files changed, 89 insertions(+), 39 deletions(-) diff --git a/src/components/CheckboxGroup/CheckboxGroupContext.jsx b/src/components/CheckboxGroup/CheckboxGroupContext.jsx index d16beb801..733a9c444 100644 --- a/src/components/CheckboxGroup/CheckboxGroupContext.jsx +++ b/src/components/CheckboxGroup/CheckboxGroupContext.jsx @@ -16,13 +16,6 @@ const CheckboxGroupProvider = ({ }) => { const parentCtx = useCheckboxGroup(); - const valuesRef = React.useRef([]); - React.useLayoutEffect(() => { - return () => { - valuesRef.current = []; - }; - }); - const onChange = parentCtx.onChange || onChangeProp; const name = parentCtx.name || nameProp; const value = parentCtx.value || valueProp; @@ -34,9 +27,9 @@ const CheckboxGroupProvider = ({ return value.includes(checkboxValue); }; - const getIsAllChecked = () => { - const hasUnchecked = valuesRef.current.some((item) => !getIsItemChecked(item)); - const hasChecked = valuesRef.current.some((item) => getIsItemChecked(item)); + const getIsAllChecked = (values) => { + const hasUnchecked = values.some((item) => !getIsItemChecked(item)); + const hasChecked = values.some((item) => getIsItemChecked(item)); return hasUnchecked && hasChecked ? 'partial' : !hasUnchecked && hasChecked; }; @@ -49,21 +42,16 @@ const CheckboxGroupProvider = ({ onChange(newValues, name, checkboxValue); }; - const onAllChange = () => { - const isAllChecked = getIsAllChecked(); + const onAllChange = (values) => () => { + const isAllChecked = getIsAllChecked(values); if (isAllChecked === true) { - const newValues = value.filter((item) => !valuesRef.current.includes(item)); + const newValues = value.filter((item) => !values.includes(item)); onChange(newValues, name); } else { - onChange(_(value).concat(valuesRef.current).uniq().value(), name); + onChange(_(value).concat(values).uniq().value(), name); } }; - const recordValue = (checkboxValue) => { - valuesRef.current.push(checkboxValue); - parentCtx.recordValue?.(checkboxValue); - }; - return { variant, value, @@ -74,10 +62,8 @@ const CheckboxGroupProvider = ({ getIsAllChecked, onItemChange, onAllChange, - valuesRef, - recordValue, }; - }, [getIsChecked, value, name, onChange, variant, parentCtx]); + }, [getIsChecked, value, name, onChange, variant]); return {children}; }; diff --git a/src/components/CheckboxGroup/index.d.ts b/src/components/CheckboxGroup/index.d.ts index 66156e5d4..c3805c0f8 100644 --- a/src/components/CheckboxGroup/index.d.ts +++ b/src/components/CheckboxGroup/index.d.ts @@ -39,6 +39,10 @@ declare const CheckboxGroupItem: React.FC; export interface CheckboxGroupAllProps { label?: React.ReactNode; className?: string; + /** + * a array of values that the All option represent + */ + values: any[]; } declare const CheckboxGroupAll: React.FC; diff --git a/src/components/CheckboxGroup/index.jsx b/src/components/CheckboxGroup/index.jsx index 9e9e49824..1b888fd60 100644 --- a/src/components/CheckboxGroup/index.jsx +++ b/src/components/CheckboxGroup/index.jsx @@ -15,11 +15,7 @@ const CheckboxGroupItem = ({ value, ...rest }) => { invariant(!rest.variant, 'CheckboxGroup.Item: variant will be overridden by CheckboxGroup variant'); invariant(!rest.onChange, 'CheckboxGroup.Item: onChange will be overridden by CheckboxGroup onChange'); - const { onItemChange, getIsItemChecked, name, variant, recordValue } = groupCtx; - - React.useLayoutEffect(() => { - recordValue(value); - }); + const { onItemChange, getIsItemChecked, name, variant } = groupCtx; return ( { CheckboxGroupItem.propTypes = { ...shareCheckboxPropTypes }; -const CheckboxGroupAll = ({ className, label = 'All', ...rest }) => { +const CheckboxGroupAll = ({ className, label = 'All', values, ...rest }) => { const groupCtx = useCheckboxGroup(); invariant(!_.isEmpty(groupCtx), 'CheckboxGroup.All: must be used as children of CheckboxGroup'); @@ -47,8 +43,8 @@ const CheckboxGroupAll = ({ className, label = 'All', ...rest }) => { className={classnames(className, 'is-all')} name={name} label={label} - checked={getIsAllChecked()} - onChange={onAllChange} + checked={getIsAllChecked(values)} + onChange={onAllChange(values)} variant={variant} /> ); @@ -57,6 +53,10 @@ const CheckboxGroupAll = ({ className, label = 'All', ...rest }) => { CheckboxGroupAll.propTypes = { label: PropTypes.node, className: PropTypes.string, + /** + * a array of values that the All option represent + */ + values: PropTypes.array.isRequired, }; const CheckboxGroup = ({ diff --git a/src/components/CheckboxGroup/index.spec.jsx b/src/components/CheckboxGroup/index.spec.jsx index 25e3d7aff..2f52161ec 100644 --- a/src/components/CheckboxGroup/index.spec.jsx +++ b/src/components/CheckboxGroup/index.spec.jsx @@ -194,7 +194,7 @@ describe('', () => { const onChange = jest.fn(); const { container } = render( - + @@ -211,7 +211,7 @@ describe('', () => { const onChange = jest.fn(); const { container } = render( - + @@ -228,7 +228,7 @@ describe('', () => { const onChange = jest.fn(); const { container } = render( - + @@ -271,7 +271,7 @@ describe('', () => { - + @@ -283,4 +283,57 @@ describe('', () => { expect(onChange).toHaveBeenCalledTimes(1); expect(onChange).toHaveBeenCalledWith(['Lung Cancer Late', 'Lung Cancer Early'], 'movies'); }); + + it('should work when there is an initial value', () => { + const onChange = jest.fn(); + const { container } = render( + + + + + + + ); + + const checkbox = getByTestIdGlobal(getByDts(container, 'target'), 'checkbox-input'); + expect(checkbox.checked).toBe(true); + }); + + it('should work when the values are updated', () => { + const Component = () => { + const [value, setValue] = React.useState([]); + const [allValues, setAllValues] = React.useState(['terminator', 'predator', 'soundofmusic']); + + return ( + + + {allValues.map((item) => ( + + ))} + + + ); + }; + + const { queryAllByTestId, getByTestId } = render(); + + const items = queryAllByTestId('checkbox'); + expect(items[0]).toHaveAttribute('aria-checked', 'false'); + + fireEvent.click(getByTestIdGlobal(items[1], 'checkbox-input')); + fireEvent.click(getByTestIdGlobal(items[2], 'checkbox-input')); + fireEvent.click(getByTestIdGlobal(items[3], 'checkbox-input')); + expect(items[0]).toHaveAttribute('aria-checked', 'true'); + + fireEvent.click(getByTestId('button')); + + expect(items[0]).toHaveAttribute('aria-checked', 'mixed'); + }); }); diff --git a/www/containers/props.json b/www/containers/props.json index e15cb5645..c659cff14 100644 --- a/www/containers/props.json +++ b/www/containers/props.json @@ -1396,6 +1396,13 @@ }, "required": false, "description": "" + }, + "values": { + "type": { + "name": "array" + }, + "required": true, + "description": "a array of values that the All option represent" } } }, @@ -1425,7 +1432,7 @@ ], "params": [ { - "name": "{ className, label = 'All', ...rest }", + "name": "{ className, label = 'All', values, ...rest }", "type": null } ], diff --git a/www/examples/CheckboxGroup.mdx b/www/examples/CheckboxGroup.mdx index 471d55bc0..18794dcd9 100644 --- a/www/examples/CheckboxGroup.mdx +++ b/www/examples/CheckboxGroup.mdx @@ -135,15 +135,15 @@ const Example = () => { className="checkbox-advanced" > - + {allHobbies.sports.map((item) => ( - + ))} - + {allHobbies.other.map((item) => ( - + ))}