Skip to content

DS-1264: feat(checkbox, checkbox-group): add required state for checkboxes #1035

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ogermain-kronos wants to merge 1 commit into
base: master
Choose a base branch
from
Open

DS-1264: feat(checkbox, checkbox-group): add required state for checkboxes #1035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions packages/react/src/components/checkbox-group/checkbox-group.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,36 @@ describe('Checkbox', () => {

expect(tree).toMatchSnapshot();
});

test('should display warning message if input is invalid and not checked', () => {
const wrapper = mountWithTheme(
<CheckboxGroup
label="Vehicule"
checkboxGroup={checkboxGroup}
required
valid={false}
id="checkbox-group-test"
/>,
);

const warning = wrapper.find('checkbox-group-test_validationAlert');
expect(warning).toBeDefined();
});

test('should hide warning message if group is invalid and one input is checked', () => {
const wrapper = mountWithTheme(
<CheckboxGroup
label="Vehicule"
checkboxGroup={checkboxGroup}
required
valid={false}
id="checkbox-group-test"
/>,
);

wrapper.find('input').at(0).simulate('change');

const warningPostCheck = wrapper.find('checkbox-group-test_validationAlert');
expect(warningPostCheck).toEqual({});
});
});
16 changes: 16 additions & 0 deletions packages/react/src/components/checkbox-group/checkbox-group.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,14 @@ exports[`Checkbox Matches the snapshot 1`] = `

<label
class="c0"
for="uuid2"
>
<input
aria-invalid="false"
aria-labelledby=""
class="c1"
data-testid="checkboxGroup-boat"
id="uuid2"
name="vehicule1"
type="checkbox"
value="boat"
Expand Down Expand Up @@ -238,11 +242,15 @@ exports[`Checkbox Matches the snapshot 1`] = `

<label
class="c0"
for="uuid3"
>
<input
aria-invalid="false"
aria-labelledby=""
checked=""
class="c1"
data-testid="checkboxGroup-plane"
id="uuid3"
name="vehicule2"
type="checkbox"
value="plane"
Expand Down Expand Up @@ -366,11 +374,15 @@ exports[`Checkbox Matches the snapshot 1`] = `
<label
class="c0"
disabled=""
for="uuid4"
>
<input
aria-invalid="false"
aria-labelledby=""
class="c1"
data-testid="checkboxGroup-car"
disabled=""
id="uuid4"
name="vehicule3"
type="checkbox"
value="car"
Expand Down Expand Up @@ -494,10 +506,14 @@ exports[`Checkbox Matches the snapshot 1`] = `

<label
class="c0"
for="uuid5"
>
<input
aria-invalid="false"
aria-labelledby=""
class="c1"
data-testid="checkboxGroup-bike"
id="uuid5"
name="vehicule4"
type="checkbox"
value="bike"
Expand Down
50 changes: 47 additions & 3 deletions packages/react/src/components/checkbox-group/checkbox-group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { ChangeEvent, VoidFunctionComponent } from 'react';
import { ChangeEvent, useState, VoidFunctionComponent } from 'react';
import styled from 'styled-components';
import { useDataAttributes } from '../../hooks/use-data-attributes';
import { Checkbox } from '../checkbox/checkbox';
import { useTranslation } from '../../i18n/use-translation';
import { useId } from '../../hooks/use-id';
import { InvalidField } from '../feedbacks/invalid-field';

const Legend = styled.legend`
font-size: 0.75rem;
Expand All @@ -10,9 +13,18 @@ const Legend = styled.legend`
padding: 0;
`;

const InvalidFieldContainer = styled.div`
margin: calc(var(--spacing-1x) * -1) 0 0 var(--spacing-1x);
padding-bottom: var(--spacing-1x);
`;

interface CheckboxProps {
id?: string;
label?: string;
checkedValues?: string[];
required?: boolean;
valid?: boolean;
validationErrorMessage?: string;
checkboxGroup: {
label: string,
name: string,
Expand All @@ -25,25 +37,53 @@ interface CheckboxProps {
}

export const CheckboxGroup: VoidFunctionComponent<CheckboxProps> = ({
id: providedId,
label,
checkedValues,
checkboxGroup,
required,
valid = true,
validationErrorMessage,
onChange,
...props
}) => {
const { t } = useTranslation('checkbox-group');
const id = useId(providedId);
const dataAttributes = useDataAttributes(props);
const dataTestId = dataAttributes['data-testid'] ?? 'checkboxGroup';

const [checkedState, setCheckedState] = useState(
new Array(checkboxGroup.length).fill(false),
);
const areAllCheckboxUnchecked = checkedState.every((e) => e === false);

const handleOnChange = (position: number): void => {
const updatedCheckedState = checkedState.map((item, index) => (index === position ? !item : item));

setCheckedState(updatedCheckedState);
};
Comment on lines +55 to +64

@meriouma meriouma Nov 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis pas certain qu'on doive gérer le state. Ça devrait être synchronisé avec les checkedValues. Ou sinon il faudrait gérer le mode controlled et uncontrolled. Là on est un peu entre les deux.
Vous en pensez quoi @pylafleur @savutsang ? Est-ce qu'on se sert tant que ça du mode uncontrolled?

@ogermain-kronos ogermain-kronos Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le message d'invalidité et l'outline rouge des checkboxes doivent disparaître lorsqu'un checkbox est selectionné. Comment est-ce que je peux m'assurer que ce comporement là se fasse sans utiliser le state?

@meriouma meriouma Feb 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

La gestion du state du checkbox group c'est un peu le bordel si on veut supporter le mode uncontrolled 🤯. Et en fait je pense qu'on devrait peut-être pas le supporter, parce que de toute façon je sais pas comment on pourrait faire fonctionner le required sans gérer le state complètement.

Comment on lines +55 to +64

@meriouma meriouma Feb 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peut-être

Suggested change
const [checkedState, setCheckedState] = useState(
new Array(checkboxGroup.length).fill(false),
);
const areAllCheckboxUnchecked = checkedState.every((e) => e === false);
const handleOnChange = (position: number): void => {
const updatedCheckedState = checkedState.map((item, index) => (index === position ? !item : item));
setCheckedState(updatedCheckedState);
};
const dirty = useRef(false);
const [checkedState, setCheckedState] = useState<[string, boolean][]>(getDefaultState(checkboxGroup));
const actualCheckedValues: [string, boolean][] = (
(checkedValues && checkboxGroup.map(({ value }) => [value, checkedValues?.includes(value) ?? false]))
|| checkedState
);
const areAllCheckboxUnchecked = actualCheckedValues.every(([_, checked]) => !checked);
const handleOnChange = (value: string, newChecked: boolean): void => {
const updatedCheckedState = actualCheckedValues.map(([checkboxValue, currentChecked]) => (
[checkboxValue, checkboxValue === value ? newChecked : currentChecked] satisfies [string, boolean]
));
dirty.current = true;
setCheckedState(updatedCheckedState);
};


return (
<>
{label && <Legend>{label}</Legend>}
{
required && !valid && areAllCheckboxUnchecked

@meriouma meriouma Feb 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
required && !valid && areAllCheckboxUnchecked
(valid === false || (required && dirty.current && areAllCheckboxUnchecked))

@meriouma meriouma Feb 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En fait je sais pas si le valid est pertinent, si on a la prop, ça devrait probablement prendre le dessus sur le state

&& (
<InvalidFieldContainer>
<InvalidField
controlId={id}
feedbackMsg={validationErrorMessage || t('validationErrorMessage')}
/>
</InvalidFieldContainer>
)
}
{checkboxGroup.map(({
defaultChecked,
disabled,
label: checkboxLabel,
name,
value,
}) => (
}, pos) => (
<Checkbox
key={`${name}-${value}`}
checked={checkedValues?.includes(value)}
Expand All @@ -52,8 +92,12 @@ export const CheckboxGroup: VoidFunctionComponent<CheckboxProps> = ({
disabled={disabled}
label={checkboxLabel}
name={name}
valid={required ? valid || !areAllCheckboxUnchecked : valid}
value={value}
onChange={onChange}
onChange={(event: ChangeEvent<HTMLInputElement>) => {
handleOnChange(pos);

@meriouma meriouma Feb 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Et de changer ici pour handleOnChange(value, event.target.checked);

onChange?.(event);
}}
/>
))}
</>
Expand Down
54 changes: 54 additions & 0 deletions packages/react/src/components/checkbox/checkbox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,60 @@ describe('Checkbox', () => {
expect(input.prop('disabled')).toBe(true);
});

test('should be required when required prop is set to true', () => {
const wrapper = mountWithTheme(<Checkbox {...defaultProps} required />);

const input = wrapper.find('input');
expect(input.prop('required')).toBe(true);
});

test('should display warning message if input is invalid and not checked', () => {
const wrapper = mountWithTheme(<Checkbox {...defaultProps} id="checkbox-test" required valid={false} />);

const warning = wrapper.find('checkbox-test');
expect(warning).toBeDefined();
});

test('should hide warning message if input is invalid and then checked', () => {
const wrapper = mountWithTheme(<Checkbox {...defaultProps} id="checkbox-test" required valid={false} />);

const warningPreCheck = wrapper.find('checkbox-test');
expect(warningPreCheck).toBeDefined();

wrapper.find('input').simulate('change');

const warningPostCheck = wrapper.find('checkbox-test');
expect(warningPostCheck).toEqual({});
});

test('should have aria-labelledby prop when warning message is displayed and input is invalid', () => {
const wrapper = mountWithTheme(<Checkbox {...defaultProps} required id="checkbox-test" valid={false} />);

const input = wrapper.find('input');
expect(input.prop('aria-labelledby')).toBe('checkbox-test');
});

test('should have empty aria-labelledby prop when input is valid', () => {
const wrapper = mountWithTheme(<Checkbox {...defaultProps} required id="checkbox-test" />);

const input = wrapper.find('input');
expect(input.prop('aria-labelledby')).toBe('');
});

test('should have aria-invalid prop set to true when warning message is displayed and input is invalid', () => {
const wrapper = mountWithTheme(<Checkbox {...defaultProps} required id="checkbox-test" valid={false} />);

const input = wrapper.find('input');
expect(input.prop('aria-invalid')).toBe(true);
});

test('should have aria-invalid prop set to false when input is valid', () => {
const wrapper = mountWithTheme(<Checkbox {...defaultProps} required id="checkbox-test" />);

const input = wrapper.find('input');
expect(input.prop('aria-invalid')).toBe(false);
});

test('matches snapshot', () => {
const tree = mountWithTheme(<Checkbox {...defaultProps} />);

Expand Down
14 changes: 13 additions & 1 deletion packages/react/src/components/checkbox/checkbox.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,24 +104,36 @@ exports[`Checkbox matches snapshot 1`] = `
>
<styled.label
hasLabel={true}
htmlFor="uuid1"
key="vehicule-boat"
>
<label
className="c0"
htmlFor="uuid1"
>
<styled.input
aria-invalid={false}
aria-labelledby=""
id="uuid1"
name="vehicule"
onChange={[Function]}
type="checkbox"
value="boat"
>
<input
aria-invalid={false}
aria-labelledby=""
className="c1"
id="uuid1"
name="vehicule"
onChange={[Function]}
type="checkbox"
value="boat"
/>
</styled.input>
<styled.span>
<styled.span
$valid={true}
>
<span
className="c2 c3"
>
Expand Down
Loading