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

[FEATURE] Ajouter l'auto-scroll au Stepper (PIX-13201) #9455

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

dlahaye
Copy link
Contributor

@dlahaye dlahaye commented Jul 5, 2024

🦄 Problème

Aujourd'hui, parcourir un Stepper peut être pénible du point de vue de l'UX. En effet, durant la navigation nous n'avons pas systématiquement un focus satisfaisant sur la nouvelle Step affichée.

🤖 Proposition

Avoir le même fonctionnement que pour les grains, c'est à dire un auto-scroll à chaque Step du Stepper qui permet d’avoir chaque nouvelle Step en haut de page.

🌈 Remarques

J'ai choisi d'extraire la logique d'auto-scroll existante pour les grains dans un service afin de rester DRY.

Création d'un modifier "did-insert"

J'ai dû créer un modifier pour remplacer le did-insert natif.
Voir explications ci-dessous.

Erreur du linter "ember/no-at-ember-render-modifiers"

L'erreur du linter ember/no-at-ember-render-modifiers est apparue au sein du composant mon-pix/app/components/module/step.gjs. Étant en gjs, j'ai dû importer didInsert via la librairie ember-modifier. C'est ce qui a généré cette erreur.

Qu'est-ce qui ne va pas avec {{did-insert}}, {{did-update}}, et {{will-destroy}} ?

Cette page du plugin eslint de Ember explique pourquoi cette alerte du linter a été créée.

tldr;
Ces modifiers avaient été implémenté temporairement pour faciliter la migration d'Octane vers Glimmer. Mais ils génèrent entre autre des effets de bords.

Que faire ?

🚨Le repository de ember-template-lint recommande d'utiliser des modifiers custom
Le repo de ember/render-modifiers le recommande aussi.

En ce sens, dans cette PR ce commit propose un modifier custom pour le did-insert.
Je l'ai nommé modifier-did-insert pour éviter toute collision avec les did-insert natifs utilisés un peu partout dans la code base actuelle

💯 Pour tester

  1. Lancer le didacticiel
  2. Naviguer au sein des Stepper et constater que l'auto-scroll se fait bien
  3. Naviguer de grain en grain et constater qu'il n'y a pas de régression sur le comportement d'auto-scroll

@dlahaye dlahaye self-assigned this Jul 5, 2024
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@dlahaye dlahaye changed the title [FEATURE] Ajouter l'_auto-scroll_ au Stepper (PIX-13201) [FEATURE] Ajouter l'auto-scroll au Stepper (PIX-13201) Jul 5, 2024
@dlahaye dlahaye force-pushed the pix-13201-stepper-auto-scroll branch 4 times, most recently from 89f5ccc to 74a49e6 Compare July 5, 2024 11:11
@dlahaye dlahaye marked this pull request as ready for review July 5, 2024 11:12
@dlahaye dlahaye force-pushed the pix-13201-stepper-auto-scroll branch 2 times, most recently from 338a71f to 161f40c Compare July 5, 2024 13:06
@dlahaye
Copy link
Contributor Author

dlahaye commented Jul 8, 2024

on devrait pouvoir retirer le premier commit car finalement on n'utilise pas le offset pour les step

});

// then
sinon.assert.calledWith(scrollStub, { top: scrollToTop, behavior: 'instant' });
Copy link
Member

Choose a reason for hiding this comment

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

Il manque le test du cas où behavior = smooth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pour passer ça, j'ai refacto le test. C'est encore confus mais j'espère que tu comprendras l'intention et qu'on trouvera un bon compromis 🙏

@dlahaye dlahaye force-pushed the pix-13201-stepper-auto-scroll branch 5 times, most recently from a364b67 to bc2c504 Compare July 10, 2024 07:38
@pix-service-auto-merge pix-service-auto-merge merged commit 01ea3ba into dev Jul 10, 2024
6 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the pix-13201-stepper-auto-scroll branch July 10, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants