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

DS-464: feat(ErrorSummary): add component #488

Merged
merged 14 commits into from
Jun 20, 2023
Merged

DS-464: feat(ErrorSummary): add component #488

merged 14 commits into from
Jun 20, 2023

Conversation

pylafleur
Copy link
Contributor

@pylafleur pylafleur commented Apr 24, 2023

PR Description

DS-464

Ajouter le composant Error Summary

L'implémentation utilise la Sectional Banner, sur laquelle il a été nécessaire d'ajouter une propriété focusable pour être conforme à la documentation du Error Summary. Aussi, le titre de la sectional banner utilise un span plutôt qu'un h2 comme prescrit. À voir si c'est quelque chose qui nécessite d'être adressé, et de quelle façon.

Aussi, le visuel de l'état focus diffère légèrement du figma. Par contre, j'ai réutilisé une fonction existante pour appliquer les styles et ça semble harmonisé avec ce qu'on a sur d'autres composantes (inputs, boutons, etc).

image
image

New component checklist

  • The new component and its tests are in the same component folder.
  • The component is unit tested and/or snapshot tested.
  • All of the relevant Storybook stories have been added to the storybook package.
  • There are no linting errors or warnings in the modified/new code.

@pylafleur pylafleur requested a review from a team as a code owner April 24, 2023 18:55
@pylafleur pylafleur marked this pull request as draft April 24, 2023 19:00
@pylafleur pylafleur marked this pull request as ready for review April 28, 2023 00:13
Copy link
Contributor

@meriouma meriouma left a comment

Choose a reason for hiding this comment

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

Très cool!

packages/react/src/i18n/translations.ts Outdated Show resolved Hide resolved
packages/react/src/i18n/translations.ts Outdated Show resolved Hide resolved
packages/react/src/i18n/translations.ts Outdated Show resolved Hide resolved
@meriouma
Copy link
Contributor

meriouma commented May 9, 2023

J'ai reviewed un peu trop vite. Je pense qu'il manque que le Enter du clavier trigger le lien quand il a le focus.

@pylafleur
Copy link
Contributor Author

J'ai reviewed un peu trop vite. Je pense qu'il manque que le Enter du clavier trigger le lien quand il a le focus.

Je me fiais sur le comportement natif du browser mais j'ai utilisé l'attribut to à la place de href 🤦
J'ai fait quelques changements supplémentaires dans une PR à part (#498) et je vais corriger celle-ci basé sur ces autres changements.

Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

Quelques commentaires ici et là sinon:

  • Il faudrait que le titre du composant soit un Heading (h2, h3...).
  • Est-ce qu'il serait possible de changer le <div> container par un élément <section> ?
  • Le contenu qui vient après le titre est wrappé dans un <div>. Le <div> n'est pas vraiment nécessaire donc si tu pouvais l'enlever ça ferait ça de moins dans le code.

@pylafleur
Copy link
Contributor Author

@LarryMatte Les différences notées (<h2>, <section> et la div de trop) viennent du fait que j'ai utilisé le Sectional Banner comme building bloc pour mon component. Est-ce que tu penses qu'il y aurait moyen de rendre les 2 structures compatibles? Est-ce que le h2 et le section auraient du sens dans le Sectional Banner?

@LarryMatte
Copy link
Contributor

@LarryMatte Les différences notées (<h2>, <section> et la div de trop) viennent du fait que j'ai utilisé le Sectional Banner comme building bloc pour mon component. Est-ce que tu penses qu'il y aurait moyen de rendre les 2 structures compatibles? Est-ce que le h2 et le section auraient du sens dans le Sectional Banner?

@pylafleur Le heading <h2> pourrait possiblement être un <h3>, ça va dépendre à quel niveau de la hiérarchie de la page le composant est. Donc le tag du heading doit pouvoir changer (h2, h3, h4 etc) donc il ne faudrait pas qu'il soit "fix". Je regarde ça pour le sectional et je te reviens.

@LarryMatte
Copy link
Contributor

Est-ce que le h2 et le section auraient du sens dans le Sectional Banner?

C'est good! tu peux utiliser un élément <section> et un heading pour le Sectional Banner

Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

LGTM

@pylafleur pylafleur merged commit 321a70b into master Jun 20, 2023
19 checks passed
@pylafleur pylafleur deleted the dev/DS-464 branch June 20, 2023 13:52
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.

None yet

3 participants