[m-popup] [m-select] Problème d'affichage lorsqu'on passe de mobile à desktop

[raphpare]

Description

L'objectif de cette PR est d'améliorer la réactivité de la prop open et des slots (header, default et footer) des composants qui utilisent le m-popup.

Reformatage du code selon les normes actuelles dans les composants suivants:

• m-popper
• m-sidebar
• m-pupup
• m-base-select
• portal.ts (mixin)

Dans le composant m-popup, la balise racine div a été retiré. Les composants m-popper en desktop et m-sidebar en mobile sont maintenant positionnés à la racine du composant. Cette modification fait en sorte que tous les snapshots des composants qui utilisent un m-popup sont réécrits.

@vidal7 Avant de merger cette PR, un développeur du projet Brio devrait tester cette branche dans leur projet pour s'assurer qu'aucune régression ne sera intégrée à Modul. Voici ce qui devra être testé:

• Tester si l'ouverture et fermeture des composants suivant fonctionnent lorsque passe de la version mobile à la version desktop:
  • [ ] m-tooltip
  • [ ] m-sidebar
  • [ ] m-dropdown
  • [ ] m-select
  • [ ] m-popup
  • [ ] m-option
  • [ ] Tous les autres composants qui utilisent le m-popup

Types de changements

• [ ] Correction de bug (sans breaking change)
• [ ] Amélioration (ajout par example une nouvelle propriété, évènement, slot ou méthode à un composant existant sans breaking change)
• [ ] Nouvelle fonctionalité (nouveau composant, directive, filtre ou service)
• [ ] Breaking change (modification à une fonctionnalités existante qui nécessite une migration remplir la section release note)
• [ ] Refactoring/ménage (sans breaking change)
• [ ] Documentation/storybook (changement à la documentation ou aux storybooks qui n'affefcte aucun package)
• [ ] Autre

Comment cela peut-il être testé?

• [ ] Test unitaire (un nouveau test unitaire à été fait)
• [ ] Storybook
• [ ] Test manuel / Sandboxes
• [ ] Autre

Inclure cette section dans les release notes

Liens internes

[vidal7]

@raphpare C'est le genre de PR qui fait peur. Il y a plusieurs fichiers modifiés. Je comprend qu'il y a plusieurs snapshots mais je crois que cette PR en fait trop large pareil. Je crois que nous aurions eu intérêt à ne pas tout faire dans la même PR. Nous aurions pu avoir une PR uniquement de reformatage de code et une autre pour améliorer la réactivité de la prop open et des slots. Ainsi, il aurait été plus facile à tester, à réviser et cela nous aurait donné plus confiance en la modification. Si possible, j'aimerais qu'à l'avenir qu'on réduise le scope d'une PR. Je préfère de loin plusieurs petites PRs qu'une grande PR qui fait tout. Qu'en penses-tu?

[vidal7]

@chuckmah est-ce qu'on pourrait avoir une release spéciale pour tester la PR 410?

[chuckmah]

@chuckmah est-ce qu'on pourrait avoir une release spéciale pour tester la PR 410?

C'est parti dist tag => PR410

[chuckmah]

@raphpare C'est le genre de PR qui fait peur. Il y a plusieurs fichiers modifiés. Je comprend qu'il y a plusieurs snapshots mais je crois que cette PR en fait trop large pareil. Je crois que nous aurions eu intérêt à ne pas tout faire dans la même PR. Nous aurions pu avoir une PR uniquement de reformatage de code et une autre pour améliorer la réactivité de la prop open et des slots. Ainsi, il aurait été plus facile à tester, à réviser et cela nous aurait donné plus confiance en la modification. Si possible, j'aimerais qu'à l'avenir qu'on réduise le scope d'une PR. Je préfère de loin plusieurs petites PRs qu'une grande PR qui fait tout. Qu'en penses-tu?

On peut la flagger "next" si on considère que le changement nécessite des tests plus poussés, Pour l'instant on a pas planifié de modul 1.3 mais si vous juger qu'on devrait le faire on peut la planifier

[raphpare]

Je suis d'accord avec toi, cependant je vais t’expliquer ce qui s’est passé. Lorsque j'avais commencé la PR, je voulais simplement changer la réactivité des props et des slots. J'ai donc commencé à modifier et mettre à jour les templates. À ce moment, si j’avais mis le formatage de côté, je n’aurais surement jamais pris le temps de faire un autre PR pour appliquer le formatage. Le code final de Modul aurait donc été de moins bonne qualité. Je suis plus d’avis que lorsque nous modifions des composants dans Modul où le formatage est « deprecated », il est préférable d’appliquer immédiatement le formatage recommander pour s’assurer d’avoir du code à jour et de s’assurer d’avoir du code plus uniforme selon les nouveaux standards.

[vidal7]

@raphpare, merci pour les explications mais je suis d'avis qu'il est préférable d'appliquer le formatage dans une PR séparée soit avant une modification ou immédiatement après une PR de modification. Je ne pense pas que d'attendre quelques jours entre les deux PRs vont changer réellement quelque chose sur la qualité du code tant que la PR de formatage soit fait. L'idée est de faciliter l'acceptation et les tests de la PR pour tous.

[vidal7]

@raphpare, @chuckmah, les tests de cette PR seront réalisés par @jfdion dans Brio.

Merci à tous.

[raphpare]

Mon explication est plutôt liée à la motivation du développeur à faire une deuxième PR pour le formatage. La plupart des dev feront qu'un seul PR sans modifier le formatage. Le code Modul risque donc d'être plus dur à maintenir par la suite.

[jfdion]

Voici le résultat des tests

Tester si l'ouverture et fermeture des composants suivant fonctionnent lorsque passe de la version mobile à la version desktop:

[x] m-tooltip [ ] m-sidebar -- aucune utilisation dans Brio [ ] m-dropdown -- aucune utilisation dans Brio dans un contexte responsive [ ] m-select -- aucune utilisation dans Brio [x] m-popup [x] m-option [x] Tous les autres composants qui utilisent le m-popup

Ça semble beau du côté de Brio. Par contre tous ces tests devraient être réalisables du côté de modul avec des test d'intégration / e2e / visual testing avec des pages dédiées

[chuckmah]

@raphpare @vidal7 J'ai déployé la PR sur http://bugfix-popup-bonifier-reactivite-prop-et-slot-ul-modul-dv01.pca.svc.ulaval.ca/storybook/index.html?path=/story/modul-components-m-popper--default-story

@raphpare Je me question a savoir qu'est qu'il y a de changé exactement par rapport a "améliorer la réactivité" Et comment peut-on le tester a l'aide de storybook. Pour l'instant je vois aucun changement entre ce qu'on a dans master et la branche. Je veux bien merger la PR mais j'aimerais plus de détails sur l'amélioration avant

Merci!

[raphpare]

L'amélioration de l'interactivité de la prop openest visible seulement dans le composant m-select lorsqu'on s'amuse et redimensionnant plusieurs fois la fenêtre de son navigateur de la largeur mobile à desktop.

Les éléments dans le code qui a été modifié pour améliorer l'interactivité de la prop open se trouvent dans le composant m-popup et dans la mixin portal.ts.

Dans le m-popup, j'ai changé la façon d'utiliser la prop openen ajoutant un watch immediate sur cette prop. J'ai également ajouté un set sur le computed popperOpen() et un autre set sur le computed sideBarOpen(). La gestion du emit('update:open') a aussi été améliorée. Les états d'ouverture et de fermeture m-popper ont donc été simplifier.

Pour la mixin portal, les changements dans le code sont semblables à ceux du m-popup. J'ai ajouté un watch immediate open en m'assurant que la valeur du open soit cohérente avec les composants qui utilise cette mixin.

Avec ces modifications, nous assurons que lorsque nous utilisons ces composants avec la prop sync.open, la valeur de la prop open est cohérente avec ce qui est affiché à l'écran.

[raphpare]

Pour l'interactivité des slots de ces composants, j'ai retiré les getters dans les fichiers ts qui utilisaient les slots. Les slots sont maintenant directement utilisés dans les templates HTML pour améliorer leur réactivité.

[chuckmah]

@raphpare Oui je vois la correction du bug avec la story du m-select lorsqu'on passe mobile / desktop bien joué .

Dans cette optique je pense qu'on devrait renommer la PR pour

[m-select] Problème d'affichage lorsqu'on passe de mobile <=> desktop

Qu'en penses-tu ?

[chuckmah]

@vidal7 @raphpare Je suis d'avis que les changements sont dur a review dans cette PR, idéalement lorsqu'on fait un gros cleanup ca serait mieux de le faire séparément d'une correction de bug/ enhancement . Il faut éviter les PR de type "bonification" ou "amélioration"... Idéalement faire une PR qui contiient juste un cleanup et ensuitre une avec la correction de bug / enhancement (qui peut partir de la branche du cleanup) de cette facon c'est clair au review on on voit si les tests de snapshot sont affecté par le cleanup ou par la correction de bug/ enhancement

Pour cette PR , je la passerait dans 1.2.11 vu qu'on a fait pas mal le tour et qu'il ne semble pas avoir de régression .