-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
7896bb8
to
d2e0ad8
Compare
@@ -4,7 +4,7 @@ import React from 'react' | |||
import PropTypes from 'prop-types' | |||
import { Icon } from 'pass-culture-shared' | |||
|
|||
import flattenErrors from './utils' | |||
import { flattenErrors } from './utils' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
il cherche dans le utils.js chez moi, j'ai écris
import flattenErrors from './utils'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testes en relancant la webapp, ca devrait le faire
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, merci
Le rendu est plus beau sur le search 👍 |
@@ -0,0 +1,31 @@ | |||
import { isSameDayInEachTimezone } from '../../../helpers' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
le test est manquant onCalendarUpdate.spec.js
.react-datepicker__triangle::before { | ||
top: -3px; | ||
} | ||
} | ||
|
||
/* ----- NAVIGATION GAUCHE/DROITE ----- */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment tu as obtenu ce résultat ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je fais défiler jusqu'au calendrier et j'ai continué le swipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne peux plus, peut-être que ça a été fixé par @g4vroche ?
@annemarie35 pour le bug de l'user sans argent |
C'est bon, j'ai bien un message d'erreur qui m'informe que je n'ai pas assez d'argent |
@@ -13,11 +13,13 @@ import { externalSubmitForm } from '../forms/utils' | |||
import BookingForm from './BookingForm' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu pourrais ajouter aussi un test sur ce fichier et le séparer en deux comme sur /signin
ça permet de tester le composant et le composant connecté
y'a une grande discussion à ce sujet reduxjs/react-redux#325
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ajoute ça
Je suis en train de faire les tests sur onCalendarUpdates (qui je pense est passé à la poubelle dans une de mes branches :'( )
Pour le when/given/then tu veux qu'on les rajoute sur les Tests Unitaires aussi ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je viens de regarder pour le split du mapStateToProps
Je suis plutot pour, néanmoins je me pose la question en cas de plusieurs composants connectés dans le même dossier
Alors j'en arrive à me dire que on devrait peut être avoir un fichier mapStateToProps
qui soit le seul lieu d'import du connect et des sélecteurs pour tous les composants d'une page/feature
Ca vaut le coup d'en discuter je pense
Dans le cas ou on garde le système de signin, comment on nomme le fichier de la vue principale pour que cela soit implicite que c'est le fichier principal d'entrée
Précédemment un fichier nommé main
était un peu ce que je décris
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pour le when/given/then tu veux qu'on les rajoute sur les Tests Unitaires aussi ?
Je trouve ça plus lisible d'avoir 3 blocs et ça m'oblige à penser à ce que j'attends dans l'assertion donc je préfère et ça reste cohérent avec les tests déjà écrit
Je suis plutot pour, néanmoins je me pose la question en cas de plusieurs composants connectés dans le même dossier
J'ai pas encore vu ce cas de figure, le signin et le share ont été séparé sans que ça change les imports. Je trouve que ça prend pas trop de temps de les remettre dans un seul fichier.
Après ça fait encore plus de fichiers dans les dossiers
Alors j'en arrive à me dire que on devrait peut être avoir un fichier mapStateToProps qui soit le seul lieu d'import du connect et des sélecteurs pour tous les composants d'une page/feature
Oui, ça peut être aussi une autre solution
Ca vaut le coup d'en discuter je pense
Oui bien d'accord, j'ai proposé ça sur share et signin (et search) surtout aussi car ça permet de tester unitairement (sinon compose + connect = galère à tester)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De ce que je tire de la discussion citée par @annemarie35 (:+1:), le plus pertinent me semble être :
- Tester les
mapsStateToProps
&mapDispatchToProps
sous forme de TU. - Tester le component déconnecté en lui passant directement les props qu'on veux
bc45a56
to
0e8eee3
Compare
- add du dossier tmp au gitignore - split functions from BookingForm to testable files - ajout des tests unitaires - remove du HOC withBookingForm - split du booking header in its own file - suppr de l'ancien calendrier antd - update des styles du calendrier - ajout des arrows gauche droite sur le calendrier - update de la position de la popup du calendrier sur la booking card - ajout de l'icone du calendrier text input quand no selected date - fix des tests snapshots sur le booking form
0e8eee3
to
48c4f43
Compare
Pour validation PR