Code Review

[apibrac]

Suite de la review Si t'es chaud on peut tenter le système changement/pull request/fermeture de l'issue

c'est à dire faire comme un process de développement en start up pour que tu test

dis moi si t'es chaud et je t'explique comment faire

[apibrac]

Première chose qui est une chose de fond, premier truc que regarde un recruteur sur ton site : il est pas minifié/uglifié/webpacké tout ça je te laisse faire des recherches sur ce que ça fait, demande moi si tu comprends pas

mais en gros n'importe qui peut aller sur ton site et récupérer tout le code donc te copier + essayer de trouver des fails + il y a trop d'info c'est pas optimisé pour les autres utilisateurs

par chance les outils de maintenant font ça presque par défault, tout ce qu'il faut c'est lui dire de le faire, et pour ça il faut dire à ton process qu'il est en mode production (et pas en mode développement où tu met pas tous ces process parce que ça sert à rien) donc je devine que ton site tourne en ce moment en mode développement

il faut que tu trouve comment faire pour le mettre en production, en général c'est mettre une variable d'environnement NODE_ENV à production

[apibrac]

https://github.com/yolanlepibrac/movie-displayer/blob/f5d91a79a6c3284e1029eed2d61b09db24fe624c/public/index.html#L22

je pense que tu peux changer ton title pour un truc plus perso (c'est le truc qui s'affiche dans les tabs)

[apibrac]

pareil profites en pour changer ton manifest https://github.com/yolanlepibrac/movie-displayer/blob/f5d91a79a6c3284e1029eed2d61b09db24fe624c/public/manifest.json

[apibrac]

Petite review de code pour finir : ton reducer https://github.com/yolanlepibrac/movie-displayer/blob/0102104474963270835639394a1a3ee5de733217/Reducers/index.js#L20

Alors toujours toujours toujours tu écris ton réducer avec un switch, comme par exemple dans la doc :

https://redux.js.org/basics/reducers

Alors oui je sais ça fonctionne bien comme ça aussi, mais dis toi que redux c'est pas vraiment une technologie, il y a rien d'insurmontable codé dedans, c'est plutôt un paradigme, une idée de comment organiser les choses Et du coup ça passe juste par des conventions, c'est la seule qu'apporte vraiment redux

Et du coup pas le choix il faut les suivre jusqu'au bout c'est comme ça (dans notre cas, toute la notion de "reducer" et de nom d'action est construite pour pouvoir faire ce switch justement)

[apibrac]

https://github.com/yolanlepibrac/movie-displayer/blob/0102104474963270835639394a1a3ee5de733217/Reducers/index.js#L2

Quand t'as plusieurs imports du même fichier tu les fait d'un coup : import { A, B } from "../C";

[apibrac]

Cette faute c'est la plus grave, c'est un NOGO

https://github.com/yolanlepibrac/movie-displayer/blob/0102104474963270835639394a1a3ee5de733217/Reducers/index.js#L57

Le state de redux c'est comme les props de react, on ne les modifie JAMAIS jamais jamais c'est interdit Ce que tu fais c'est créer un nouveau state (souvent à partir de l'ancien) et tu renvoie ce nouveau state à la fin (c'est à ça que ça sert) C'est ensuite redux qui gère le fait de remplacer l'ancien par le nouveau

Mais toi même pour construire le nouveau tu ne dois surtout pas modifier l'ancien, juste t'as pas le droit

On pourra discuter des multiples raisons mais tu peux considérer ça comme une des règles absolu de react (transposée à redux dans ce cas)

Tu le fait un peu partout dans ton reducer donc tu peux le réécrir je pense

à toi de construire le nouveau state sans modifier l'ancien, tu peux utiliesr du JS normal, copier entièrement l'ancien pour modifier la copie (pas beau), ou encore utiliser des libs qui permettent de gérer les immutable (c'est leur petit nom) immutableJS ou autre à toi de chercher ce que tu préfère, mais pas sûr que tu fasse assez d'opérations pour qu'une nouvelle lib soit utile

[apibrac]

https://github.com/yolanlepibrac/movie-displayer/blob/0102104474963270835639394a1a3ee5de733217/Reducers/index.js#L70

C'est tentant mais t'as pas le droit de modifier le localStorage dans le reducer

https://stackoverflow.com/questions/35305661/where-to-write-to-localstorage-in-a-redux-app

T'as plusieurs moyens à toi de choisir

[apibrac]

Et enfin je pense que ton reducer fait des choses très différentes Genre ADD_CATEGORY et RESET_CATEGORY ça a l'air d'être le même sujet, les autres pas sûr du tout

Dans ce cas là il faut avoir plusieurs reducers et les fusionner dans ce qui est à juste titre le root reducer

Par exemple en utilisant https://redux.js.org/recipes/structuring-reducers/using-combinereducers

ça s'est moins prio

[yolanlepibrac]

Yes jsuis carrément chaud. Du genre, je fais des pull request et tu les valides ?

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail Garanti sans virus. www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

Le lun. 19 août 2019 à 18:24, Pib notifications@github.com a écrit :

Suite de la review Si t'es chaud on peut tenter le système changement/pull request/fermeture de l'issue

c'est à dire faire comme un process de développement en start up pour que tu test

dis moi si t'es chaud et je t'explique comment faire

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/yolanlepibrac/movie-displayer/issues/2?email_source=notifications&email_token=ALAWDLZVR2Z2KQXZOBPDVKLQFLCM7A5CNFSM4INDPT5KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HGA6PIA, or mute the thread https://github.com/notifications/unsubscribe-auth/ALAWDL3Q4RIPDGUH4LV2QW3QFLCM7ANCNFSM4INDPT5A .

[yolanlepibrac]

En fait, le principe de minifier, c'est pas de rendre illisible ? Quelque part,c'est pas un problème qu'on cherche les failles, ou qu'on copy mon site. Si je le rends illisible, comment quelqu'un qui y jette un coup d'oeil, voit si le site est bien fait ou pas ? Comme tu fais toi

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail Garanti sans virus. www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

Le lun. 19 août 2019 à 18:33, Pib notifications@github.com a écrit :

Première chose qui est une chose de fond, premier truc que regarde un recruteur sur ton site : il est pas minifié/uglifié/webpacké tout ça je te laisse faire des recherches sur ce que ça fait, demande moi si tu comprends pas

mais en gros n'importe qui peut aller sur ton site et récupérer tout le code donc te copier + essayer de trouver des fails + il y a trop d'info c'est pas optimisé pour les autres utilisateurs

par chance les outils de maintenant font ça presque par défault, tout ce qu'il faut c'est lui dire de le faire, et pour ça il faut dire à ton process qu'il est en mode production (et pas en mode développement où tu met pas tous ces process parce que ça sert à rien) donc je devine que ton site tourne en ce moment en mode développement

il faut que tu trouve comment faire pour le mettre en production, en général c'est mettre une variable d'environnement NODE_ENV à production

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/yolanlepibrac/movie-displayer/issues/2?email_source=notifications&email_token=ALAWDLYTEWRIA76B5RMEV3DQFLDNXA5CNFSM4INDPT5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TRIYQ#issuecomment-522654818, or mute the thread https://github.com/notifications/unsubscribe-auth/ALAWDL7CC4KIFT73576GXB3QFLDNXANCNFSM4INDPT5A .

[apibrac]

Yes jsuis carrément chaud. Du genre, je fais des pull request et tu les valides ? <avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> Garanti sans virus. www.avast.com <avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2> Le lun. 19 août 2019 à 18:24, Pib notifications@github.com a écrit : … Suite de la review Si t'es chaud on peut tenter le système changement/pull request/fermeture de l'issue c'est à dire faire comme un process de développement en start up pour que tu test dis moi si t'es chaud et je t'explique comment faire — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2?email_source=notifications&email_token=ALAWDLZVR2Z2KQXZOBPDVKLQFLCM7A5CNFSM4INDPT5KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HGA6PIA>, or mute the thread <github.com/notifications/unsubscribe-auth/ALAWDL3Q4RIPDGUH4LV2QW3QFLCM7ANCNFSM4INDPT5A> .

Good tu sais faire ? Sinon je te met les étapes

Et dans ta PR tu mets fixes #2 comme ça cette issue sera liée automatiquement, elle sera "resolved" quand la PR sera mergée

EDIT: je te met les étapes dans un commentaire en dessous au cas où, comme ça tu peux avancer

[apibrac]

En fait, le principe de minifier, c'est pas de rendre illisible ? Quelque part,c'est pas un problème qu'on cherche les failles, ou qu'on copy mon site. Si je le rends illisible, comment quelqu'un qui y jette un coup d'oeil, voit si le site est bien fait ou pas ? Comme tu fais toi <avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> Garanti sans virus. www.avast.com <avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2> Le lun. 19 août 2019 à 18:33, Pib notifications@github.com a écrit : … Première chose qui est une chose de fond, premier truc que regarde un recruteur sur ton site : il est pas minifié/uglifié/webpacké tout ça je te laisse faire des recherches sur ce que ça fait, demande moi si tu comprends pas mais en gros n'importe qui peut aller sur ton site et récupérer tout le code donc te copier + essayer de trouver des fails + il y a trop d'info c'est pas optimisé pour les autres utilisateurs par chance les outils de maintenant font ça presque par défault, tout ce qu'il faut c'est lui dire de le faire, et pour ça il faut dire à ton process qu'il est en mode production (et pas en mode développement où tu met pas tous ces process parce que ça sert à rien) donc je devine que ton site tourne en ce moment en mode développement il faut que tu trouve comment faire pour le mettre en production, en général c'est mettre une variable d'environnement NODE_ENV à production — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2?email_source=notifications&email_token=ALAWDLYTEWRIA76B5RMEV3DQFLDNXA5CNFSM4INDPT5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TRIYQ#issuecomment-522654818>, or mute the thread <github.com/notifications/unsubscribe-auth/ALAWDL7CC4KIFT73576GXB3QFLDNXANCNFSM4INDPT5A> .

Exactement En fait c'est pas le but de ton site lui même de montrer le code, le but c'est de montrer ce que tu sais faire en terme de site (enfin c'est mon avis, le tiens est défendable en vrai) C'est le but de ton repo (qui est public) de montrer le code Donc sur le repo "les gens" peuvent voir le code + tout ce qu'il y a autour genre server, scripts de déploiement config et tout Par contre sur ton site tu fais comme devrait être un site, donc minifier toussa

[apibrac]

Etapes pour le process de dev en équipe : par validation de PR

Tu te met sur ta branche develop et tu prends la dernière version

git checkout develop
git pull

Tu créé une nouvelle branch à partir de là, par exemple "minification", et tu te met dessus

git checkout -b minification

Tu dev tranquillou et tu commit les changement

git commit -am "Je tente de minifier"
git add .
git commit -m "nouveaux fichiers à minifier"
git commit -am "voilà c'est minifié"

tu push ta branch sur github

git push origin minification

sur github tu créé une pull request : pull requests > new > choix de la branch de départ (develop) > choix de la branch d'arrivée (minification) il y a aussi un raccourci vert qui apparait tout en haut quand tu viens de push une branch

tu met un titre (par exemple Minification de l'app) et un assignee (toi)

tu met un descriptif pour que plus tard tu puisse comprendre à quoi ça servait ces changement. Dans ce cas précis tout est dans une issue donc t'as juste à la référencer en mettant #2 (parce que c'est l'issue num 2) Et plus que ça ta PR est censé résoudre tous les problème de l'issue donc tu met fixes #2 comme ça une fois que la PR est finie l'issue est resolved et close automatiquement

et très important, tu regarde l'onglet "Files changed" comme ça tu contrôle ce que tu soumet si c'est pas bon tu refet des commit chez toi et tu re-push c'est tout

quand ça te parait bon tu met un reviewer (moi en l'occurence) à partir de là je suis notifié et je prend le relai