Skip to content
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.

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

chore(form-field): form field and input refactor to provide context #857

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

hebernardEquisoft
Copy link
Contributor

@hebernardEquisoft hebernardEquisoft commented Apr 26, 2024

DS-924

Description

Ici on vient faire un léger refactor de FormField afin qu'il puisse provide du context aux inputs enfants.
Dans l'optique de mieux gérer la relation entre label et aria-label au niveau de l'accessibilité.

Si cette approche est apprové, il va rester à faire la migration sur chacune des composantes de controls.

Ma tentative précédente.

Tests fonctionnels

  • Valider le bon fonctionnement des inputs et des critères d'accessibilité

@hebernardEquisoft hebernardEquisoft requested a review from a team as a code owner April 26, 2024 18:58
Copy link

Storybook for this build: https://ds.equisoft.io/pr-857/

Copy link

Webapp for this build: https://ds.equisoft.io/pr-857/webapp/

Copy link
Contributor

@mmorin-equisoft mmorin-equisoft left a comment

Choose a reason for hiding this comment

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

WIP (je poursuis plus tard)

packages/react/src/components/form/form-field-context.ts Outdated Show resolved Hide resolved
packages/react/src/components/form/utils.ts Outdated Show resolved Hide resolved
packages/react/src/components/form/utils.ts Outdated Show resolved Hide resolved
packages/react/src/components/form/form-field-context.ts Outdated Show resolved Hide resolved
packages/react/src/components/text-input/styles/inputs.tsx Outdated Show resolved Hide resolved
Comment on lines 151 to 152
aria-label={ariaLabel}
aria-labelledby={ariaLabel ? undefined : ariaLabelledby}
Copy link
Contributor

Choose a reason for hiding this comment

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

aria-labelledby takes precedence over all other methods of providing an accessible name, including aria-label, label and the element's inner text.
mdn webdocs - aria-labelledby

J'ignore si ça été discuté dans l'autre PR
On serait pas mieux de garder la même logique et d'affecter aria-label seulement si il n'y a pas de aria-labelledby ou juste de pas conditionner la propriété?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je crois que selon ce que j'avais discuté avec @LarryMatte et surtout implémenté dans mes tests du form c'est qu'on veut que le aria-labelledby soit overridé par aria-label s'il y en a un de providé.
S'alignant dans l'ordre d'idée que label/aria-labelledby et aria-label sont mutuellement exclusifs!

packages/react/src/components/form/utils.ts Outdated Show resolved Hide resolved
}
`;

export const FieldContainer: FunctionComponent<PropsWithChildren<FieldContainerProps>> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

[Question]
Devrait-il toujours s'appeler FieldContainer ? Tu as changé le nom du dossier pour field, donc le component devrait se nommer maintenant Field.
Qu'est-ce que tu en penses ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants