HE-Arc / Technocrarc

Web app, to edit music
2 stars 1 forks source link

Remarques #83

Open Ishydo opened 4 years ago

Ishydo commented 4 years ago

Salut à vous ! :smile:

Quelques remarques préliminaires après un petit survol de votre code et test de votre projet en local. Sachant que c'est encore en cours de dev, je n'ai pas tout analysé en détail, il y a donc peut-être des éléments sur lesquels vous travaillez déjà. Toutes ces infos sont à titre indicatif.

Bravo pour l'utilisation de technos variées (setup redis/celery, dev frontend avec dépendances, materialize :heart_eyes: ). Vous exploitez djangorestframework correctement pour arriver à un bon exercice de création d'API, c'est top. Vous montrez une bonne compréhension de django + restframework et utilisation des vues génériques.

On voit que le projet est travaillé :smile: Mes remarques concernent donc principalement des bonnes pratiques.

Gestion de vos dépendances frontend

Je me permets donc tout de même une petite remarque sur la gestion de vos dépendances frontend. Vous avez opté pour le téléchargement des ressources et le stockage en local "statique" (vos fichiers materialize.min.js / materialize.min.css / wavesurfer.js). C'est une façon de faire mais cela va, d'une part, alourdir considérablement votre repo en fichiers de dépendances au fur et à mesure que le projet grandira. De plus, vous risquez d'être embêtés et manquerez de souplesse si vous souhaitez mettre une (ou toutes) ces dépendances à jour. Je ne doute pas du fait que vous connaissez l'existence du package manager npm, qui vous permettrait d'alléger votre repo tout en vous permettant de gérer facilement ces dépendances et leur mise à jour.

Ainsi vous pourriez faire des npm install materialize-css ou npm install wavesurfer.js et avoir un fichier de descripttions de vos dépendances auto-généré : package.json.

Finalement, vous l'avez fait pour la partie python (pip install et requirements.txt alors pourquoi ne pas le faire pour la partie js :wink: ). Si ça ne vous parle pas du tout : petit lien.

Pour rappel, sur ce coup il n'est pas nécessaire de mettre tout ça en place puisque cela prendrait un certain temps en setup (mais c'est un grand plus si vous trouvez le temps!), et l'utilisation des autres technos dans le cadre de votre projet compense largement ce petit manque. Mais je vous recommande fortement de garder cela en tête pour les prochains projets car vous risquez fort d'avoir à vous frotter à npm dans le monde extérieur.

C'est ma seule remarque "importante" concernant la structure de votre projet, pour le reste, tout roule super bien à priori. Pour le reste, on pourrait être tatillon sur le style guide python (ordre imports, nom de certaines variables, ..)

Password field laggy ?

Et autre détail pendant que j'y pense, en local, en voulant me créer un compte sur Technocrarc, le champ password du formulaire d'inscription était buggy. Le focus s'enlevait et se remettait en boucle. J'ai cu copier coller mon mot de passe pour m'en sortir. C'est peut-être un problème lié à mon environnement mais je vous le signale quand même.

à dispo si besoin

Je vous souhaite une excellente continuation ! :smile: :+1: :+1:

JonasFreibur commented 4 years ago

Merci beaucoup d'avoir pris le temps de nous écrire ces remarques très pertinentes. L'utilisation de npm est une très bonne idée que j'appliquerai dans mes prochains projets. Bonne journée.

Zegorax commented 4 years ago

Merci beaucoup pour vos remarques! Concernant le password field, je n'ai pas vu ça chez moi ou sur la version déployée, si j'arrive à le reproduire je jetterai un oeil!

qtipee commented 4 years ago

Merci beaucoup !

Je ne pense pas que nous aurons le temps de mettre en place npm pour nos dépendances d'ici la présentation de mercredi, mais comme le rendu est dans plusieurs semaines, ça peut être intéressant de le faire :)