-
Notifications
You must be signed in to change notification settings - Fork 0
j'espere que c'est bien claire maintenant #2
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
base: main
Are you sure you want to change the base?
Conversation
Chouchen
left a comment
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 cahier des charges est relativement bien respecté et le rendu est propre. Il manque que les vérifications de sécurité et quelques petits problèmes de migration. J'ai déjà envoyé cette review à M. Belhomme.
| @@ -0,0 +1,35 @@ | |||
| <?php | |||
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.
Pourquoi y a-t-il 3 fichiers de migrations relativement similaires ? En partant d'un projet "neuf", la migration ne passe pas à cause de ça (ça essaye de créer 3 fois les mêmes tables).
public/assets/js/comments.js
Outdated
| @@ -0,0 +1,2 @@ | |||
|
|
|||
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.
Fichier vide ?
public/assets/js/main.js
Outdated
| Free for personal and commercial use under the CCA 3.0 license (html5up.net/license) | ||
| */ | ||
|
|
||
| /*const commentForm = document.getElementById('#comment-form'); |
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.
Pourquoi ne pas avoir laissé le code ici ? Ce serait plus propre.
| { | ||
| $builder | ||
| ->add('email',EmailType::class,[ | ||
| 'label' => 'Ajouter votre email', |
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.
Ce champ ne devrait pas être obligatoire
src/Form/CommentsType.php
Outdated
| 'attr' => [ | ||
| 'placeholder' => 'titre', | ||
| 'class' => 'title', | ||
| 'minlength' => 4 |
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.
Ceci ne valide que la longueur du champ HTML, on peut toujours passer outre en modifiant dans la console ;)
Il faudrait passer par des contraintes (constraint sur symfony)
…fichage des erreurs et de résussite
No description provided.