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

Régression autocomplete #156

Merged
merged 3 commits into from
May 6, 2021
Merged

Régression autocomplete #156

merged 3 commits into from
May 6, 2021

Conversation

fcamblor
Copy link
Collaborator

@fcamblor fcamblor commented May 5, 2021

J'ai détecté 2 régressions sur l'autocomplete :

  • Une fois que la dropdown est affichée, le filtrage ne se fait plus sur les caractères saisis ensuite (c'était lié au cleanup, lors de d1a847b)
  • Les départements ne sont plus affichés dans la dropdown (lié au cleanup d1a847b)
  • Lorsqu'on saisissait un code département, à la fois le département et la première commune étaient en aria-selected=true

@fcamblor fcamblor requested a review from Floby May 5, 2021 23:13
@fcamblor fcamblor changed the title Régression cleanup autocomplete Régression autocomplete May 5, 2021
@fcamblor fcamblor marked this pull request as ready for review May 5, 2021 23:44
@@ -318,7 +320,7 @@ export class VmdCommuneOrDepartmentSelectorComponent extends LitElement {
return html`<li
class="autocomplete-result"
role="option"
aria-selected="${index === 0}"
aria-selected="${index === 0 && this.departementsAffiches.length === 0}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi ne pas gérer l'index dans la méthode renderListItems ? Cela évite de mettre un sac de nœuds entre toutes les méthodes privées.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

J'ai fait au plus vite en analysant avant/après le merge/rebase

Mais je suis tout à fait d'accord qu'il serait plus intéressant de faire transpirer cette notion à travers les 2 render

@Floby
Copy link
Collaborator

Floby commented May 6, 2021

Hello et merci @fcamblor pour le fix (et désolé pour la régression que j'ai introduite 😞)
Même si c'est une PR plus grosse, je voulais faire remarquer que la PR #148 corrige également ce problème avec l'avantage de fournir 2 suites de tests :

  • sur le composant en lui-même, y compris la navigation au clavier
  • sur le service qui gère l'autocomplétion et la récupération des données

@Floby
Copy link
Collaborator

Floby commented May 6, 2021

Du coup ce que je voulais dire avec mon commentaire précédent, c'est que, sauf urgence MEP, on pourrait peut-être merger préférablement la #148 plutôt que celle-ci.

@fcamblor
Copy link
Collaborator Author

fcamblor commented May 6, 2021

Je préssens un peu qu'on doive MEP aujourd'hui, je préfèrerais merger cette PR afin d'avoir un dev clean.

Sauf si ça te fait des conflits dans tous les sens de rebase #148 par-dessus celle-ci ?

@Floby
Copy link
Collaborator

Floby commented May 6, 2021

du coup on merge celle-ci, je me demerderai avec les conflits

@Floby
Copy link
Collaborator

Floby commented May 6, 2021

les conflits avec le harnais de test ça me fait moins peur :)

@Luwangel Luwangel merged commit 7b193f4 into dev May 6, 2021
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.

3 participants