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

feat(Select): Ajoute un component select dropdown avec une fonctionnalité de recherche #98

Merged
merged 77 commits into from
May 12, 2020

Conversation

maxime-gendron
Copy link
Contributor

@maxime-gendron maxime-gendron commented Dec 9, 2019

PR Description

Ce PR ajoute le dropdown et la fonctionnalité de recherche au component select. Ce component utilise list comme menu Dropdown.

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.
  • The CircleCI build is successful.

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.

  • Après avoir fermé le listbox, le focus devrait rester sur l’input.
  • Mon commentaire sur le fait de mettre le padding sur le StyledInput est que ton focus devrait être sur ton input et non sur la div qui le contient.
  • Si tu commences à entrer une valeur et que ça correspond à une option, la première option devrait être sélectionné automatiquement.
  • Si tu commences à entrer une valeur dans l'input et que tu ne fais pas de sélection, lorsque l'utilisateur clique à l'extérieur du combobox, la valeur devrait rester dans le champ texte. Présentement ça retourne au placeholder

Touche du clavier:

  • Le listbox devrait pouvoir se fermer lorsque l'utilisateur utilise la touche Esc du clavier
  • Si l'utilisateur utilise le touche Esc pour fermer le listbox, le focus devrait être sur l'input et la sélection devrait être vide.
  • Mon erreur pour ce qui suit, j’aurais dû le vérifier dans la PR sur la listbox de Julie mais si l'utilisateur utilise la touche du clavier flèche du bas pour naviguer à travers les options, lorsqu'il arrive à la dernière option de la liste, ca devrait retourner à la première option. Même chose pour la flèche du haut, s'il clique sur la flèche du haut et qu'il est sur la première option, ca devrait tomber sur la dernière option de la liste.

Sur la div tu devrais avoir:
role: combobox
aria-haspopup=“listbox”
aria-expanded=“false | true”
aria-owns=“IDREF”

Sur l’input text à l’intérieur du div tu devrais avoir:
role: textbook ou searchbox
aria-autocomplete=“list”
aria-multiline=“false”
aria-controls=“IDREF”

Pour avoir plus de détails là dessus regardes:
https://www.w3.org/TR/wai-aria-1.1/#combobox
https://www.w3.org/TR/wai-aria-practices-1.1/examples/combobox/aria1.1pattern/listbox-combo.html

Sinon, je me demande si le combobox devrait vraiment inclure le select. je cherche là dessus

packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/list/list.tsx Outdated Show resolved Hide resolved
packages/react/src/components/list/list.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
@maxime-gendron maxime-gendron force-pushed the dev/DE-68 branch 2 times, most recently from 626af4a to 21e534b Compare December 18, 2019 14:09
@maxime-gendron maxime-gendron force-pushed the dev/DE-68 branch 4 times, most recently from 5b305c0 to ff91c64 Compare December 20, 2019 15:24
@maxime-gendron
Copy link
Contributor Author

@jni- j'ai répondu à tes commentaires tu me diras si c'est all good.

packages/react/src/components/list/list.tsx Outdated Show resolved Hide resolved
packages/react/src/components/list/list.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@jni- jni- left a comment

Choose a reason for hiding this comment

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

@maxime-gendron quelques autres commentaires. Au minimum le L majuscule a corriger je dirais, sinon all good donc j'te laisse le soin de checker ca

packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
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.

Je viens de refaire le tour, ce qu'il te reste:

  • Si tu commences à entrer une valeur et que ça correspond à une option, la première option devrait être sélectionné automatiquement.
  • Le cursor n'est pas visible lorsque nous écrivons.
  • Si j'écris du texte, que je sors de l'input et que j'y retourne, je ne peux pas recommencer à écrire,
  • Si je fait une selection et que je retourne dans l'input, je ne peux pas recommencer à écrire
  • Si je sélectionne un element de la liste, que je ferme le dropdown, lorsque je retourne sur l'input, le dropdown devrait s'ouvrir mais afficher seulement l'item sélectionné. Présentement il affiche toute la liste
  • Mon erreur pour ce qui suit, j’aurais dû le vérifier dans la PR sur la listbox de Julie mais si l'utilisateur utilise la touche du clavier flèche du bas pour naviguer à travers les options, lorsqu'il arrive à la dernière option de la liste, ca devrait retourner à la première option. Même chose pour la flèche du haut, s'il clique sur la flèche du haut et qu'il est sur la première option, ca devrait tomber sur la dernière option de la liste.

Sur l’input text à l’intérieur du div tu devrais avoir:

role: textbook ou searchbox
aria-autocomplete=“list”
aria-multiline=“false”
…

@@ -16,6 +16,10 @@ interface ListOption extends Option {
}

interface ListProps {
/**
* Sets list id
Copy link
Contributor

Choose a reason for hiding this comment

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

Ça me semble évident que la prop listId ça set l'id de la liste, mais concrètement ça sert à quoi? Faudrait que s'il y a un commentaire, il aide le dev à décider s'il doit mettre une valeur ou pas. Sinon name est assez fréquent pour les inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'avais ajouté listId, car il y avait des problèmes de rerender lorsqu'on passait de nouvelles valeurs dans option[]. Le problème était qu'on définissait quelle option était selected dépendant de son option.id qui correspondait à index_option.value, ce qui brisait lorsque l'option changeait d'index au rerender.

J'ai changé la logique et maintenant le option.id est ${id}_${option.value} et le id a une default value de id = uuid.v4().

packages/react/src/components/select/select.test.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/react/src/components/select/select.tsx Outdated Show resolved Hide resolved
@maxime-gendron maxime-gendron force-pushed the dev/DE-68 branch 2 times, most recently from f35589b to af7063d Compare January 13, 2020 20:46
@maxime-gendron maxime-gendron force-pushed the dev/DE-68 branch 6 times, most recently from ceea6ee to ed1bcc6 Compare January 21, 2020 17:58
@maxime-gendron maxime-gendron merged commit 0e0c7bb into master May 12, 2020
@maxime-gendron maxime-gendron deleted the dev/DE-68 branch May 12, 2020 15:18
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.

4 participants