DS-1264: feat(checkbox, checkbox-group): add required state for checkboxes #1035
Conversation
|
Storybook for this build: https://ds.equisoft.io/pr-1035/ |
|
Webapp for this build: https://ds.equisoft.io/pr-1035/webapp/ |
| @@ -116,49 +156,75 @@ export const Checkbox: FunctionComponent<PropsWithChildren<CheckboxProps>> = for | |||
| disabled, | |||
| id, | |||
There was a problem hiding this comment.
Tu devrais faire comme on fait dans d'autres components
| id, | |
| id: providedId, |
| const checkboxRef = useRef<HTMLInputElement>(null); | ||
| const [isChecked, setIsChecked] = useState<boolean | undefined>(checked || false); | ||
| const validationAlertId = `${id || uuid()}_validationAlert`; |
There was a problem hiding this comment.
Et utiliser le hook useId. En gros ça fait un useMemo de uuid() si jamais providedId est undefined
const id = useId(providedId);
| const checkboxRef = useRef<HTMLInputElement>(null); | ||
| const [isChecked, setIsChecked] = useState<boolean | undefined>(checked || false); |
There was a problem hiding this comment.
Tu devrais pas tracker ça dans le state je crois. Si la prop checked est modifiée, le state interne sera plus valide.
There was a problem hiding this comment.
Ça serait peut être intéressant de faire un shared custom hook qui vient gérer les différents uncontrolled/controlled props? 🤔
Mettons pour checked, value, valid...
On pourrait uniformiser le comportement de ces props au travers nos différents composantes d'inputs!
| @@ -116,49 +156,75 @@ export const Checkbox: FunctionComponent<PropsWithChildren<CheckboxProps>> = for | |||
| disabled, | |||
| id, | |||
| indeterminate, | |||
| isInGroup = false, | |||
There was a problem hiding this comment.
Je suis pas certain de cette prop. Si la checkbox est dans un group, il faudrait juste pas mettre de required sur les checkbox individuels il me semble?
| defaultChecked={defaultChecked} | ||
| <> | ||
| { | ||
| required && !valid && !isChecked && !isInGroup |
There was a problem hiding this comment.
Ça devrait juste être !valid je pense la condition
|
|
||
| useImperativeHandle(ref, () => checkboxRef.current, [checkboxRef]); | ||
| const dataAttributes = useDataAttributes(otherProps); | ||
|
|
||
| useEffect(() => { | ||
| if (checkboxRef.current) { | ||
| checkboxRef.current.indeterminate = indeterminate || false; | ||
| checkboxRef.current.checked = checked || false; |
There was a problem hiding this comment.
Je pense pas que ce soit nécessaire vu que la prop checked est passée à l'input
| { | ||
| required && !valid && areAllCheckboxUnchecked | ||
| && ( | ||
| <ValidationErrorAlert id={validationAlertId} label={label || ''}> |
There was a problem hiding this comment.
Et le message semble affiché dès le load du component. Je pense qu'on voulait pas ça, on l'avait enlevé sur le TextInput il me semble. @LarryMatte ?
There was a problem hiding this comment.
Le message d'erreur devrait s'afficher si l'utilisateur n'a pas sélectionné de checkbox et qu'il submit le formulaire. La validation va fail et le message d'erreur va s'afficher.
| 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); | ||
| }; |
There was a problem hiding this comment.
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?
| />, | ||
| ); | ||
|
|
||
| const warning = wrapper.find('div#checkbox-group-test_validationAlert'); |
There was a problem hiding this comment.
Je pense que t'as pas besoin du div dans le sélecteur?
| const warningPreCheck = wrapper.find('div#checkbox-group-test_validationAlert'); | ||
| expect(warningPreCheck).toBeDefined(); |
There was a problem hiding this comment.
Pas besoin de ce bout là, ton test précédent fait la même chose.
There was a problem hiding this comment.
Au niveau visuel, tout baigne! Par contre, j'ai l'impression que lorsqu'un checkbox est sélectionné, les autres checkboxes du groupe devrait passer à l'état "Default" plutôt que de rester en erreur. Cet exemple de MUI illustre un peu ce que je veux dire. Lorsque la consigne est atteinte, le champ n'est plus en erreur ce qui renvoie un feedback positif à l'utilisateur. @LarryMatte qu'en penses-tu?
effectivement |
https://equisoft.atlassian.net/browse/DS-1264
Single checkbox
Checkbox group
