Skip to content
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

Intégration de Gulp et de bower (#771) #777

Merged
merged 11 commits into from Jun 7, 2014

Conversation

sandhose
Copy link
Contributor

@sandhose sandhose commented Jun 5, 2014

Q R
Correction de bugs ? non
Nouvelle Fonctionnalité ? oui
Tickets concernés #771

Intégration de Gulp et de Bower pour le front, plus qu'a updater le README, rajouter une tâche "watch" (pour mettre à jour les fichiers à la volée), et mettre à jour tous les fichiers JS pour les conformer à JSHint (j'ouvrirais une autre issue pour se fixer sur les options)

@Eskimon
Copy link
Contributor

Eskimon commented Jun 5, 2014

Je vais probablement passer pour le casse pied mais quand je vois:

plus qu'a updater le README

...je me permert de demander "pourquoi dans ce cas le README est pas update dans la PR ?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c4acea1 on sandhose:feature-771-gulp into 0a1a360 on zestedesavoir:beta-0.2.

@sandhose
Copy link
Contributor Author

sandhose commented Jun 5, 2014

Voila voila ;)

(il est fait à l'arrache par manque de temps, je détaillerais plus dans la soirée)

@Eskimon
Copy link
Contributor

Eskimon commented Jun 5, 2014

Je vais faire le gros casse pied mais : Tu ne bosses pas sur une fonction bloquante ou un truc du genre, du coup c'est pas grave si tu lances ta PR maintenant ou demain matin, donc tu as le temps de peaufiner le README :D

(oui je suis chiant, mais j'ai le droit c'est mon petit @sandhose à moi :) )

@Alex-D
Copy link
Contributor

Alex-D commented Jun 5, 2014

Je vais être encore plus chiant :

Intégration de Gulp et de Bower

Or, le CONTRIBUTING indique de ne faire un commit par modification logique (genre là, 1 pour Gulp et 1 pour Bower).

Là on se retrouve avec un commit fourre-tout alors que c'est quand même des modifications assez importantes pour le dev.

En tous cas, tant qu'il n'y a pas de procédure détaillée sur l'installation Windows et Linux (j'épargne Mac, peu de dev du projet sur Mac, mais si tu le fais on ne dira pas non ^^).

Voilà voilà, donc pour cette fois ci c'est pas très grave mais fais attention pour les prochaines PR :)

@Alex-D
Copy link
Contributor

Alex-D commented Jun 5, 2014

Je viens de tout regarder j'ai quelques remarques :

  • il n'y a aucune explication quant à l'installation de compass : est-ce que c'est automatique ? géré ? Il me semblait que gulp-compass avait comme besoin d'avoir compass (et ruby et tout) d'installé en local.
  • il faut rassembler les vendors et les custom en un seul fichier pour la prod : minimisons le nombre de requête au maximum
  • l'idéal serait d'avoir tous les JS séparés en dev pour mieux déboguer (ou si tu sais m'expliquer comment savoir quel erreur est dans quel fichier de dev, je prends aussi).
  • pourquoi mettre les js dans un dossier js/main/ ? A priori il n'y a rien d'autre non ? Autant mettre directement dans js/ ou je me trompe ?
  • Readme à revoir un minimum en effet
  • Le watch est indispenssable, d'ailleurs si tu peux coupler ça à Livereload ou une extension Chrome-compatible ça serait le top. Actuellement j'utilise Prepros, si je merge ton travail (qui est bien cool !) je ne pourrais plus l'utiliser, il faut donc que je retrouve au minimum autant de confort.

Je crois que c'est tout :)

@geoffreyc
Copy link
Contributor

C'est tres subjectif ca. Pour moi c'est un tout gulp et bower, ils se
complémentent. Ca sert a rien de faire des commits pour le plaisir des
commits.

Si il avait fait une module de t'chat et un module de jeu d’échecs en un
seul commit ça aurait été différent.

2014-06-05 20:08 GMT+01:00 Alexandre Demode notifications@github.com:

Je vais être encore plus chiant :

Intégration de Gulp et de Bower

Or, le CONTRIBUTING indique de ne faire un commit par modification logique
(genre là, 1 pour Gulp et 1 pour Bower).

Là on se retrouve avec un commit fourre-tout alors que c'est quand même
des modifications assez importantes pour le dev.

En tous cas, tant qu'il n'y a pas de procédure détaillée sur
l'installation Windows et Linux (j'épargne Mac, peu de dev du projet sur
Mac, mais si tu le fais on ne dira pas non ^^).

Voilà voilà, donc pour cette fois ci c'est pas très grave mais fais
attention pour les prochaines PR :)


Reply to this email directly or view it on GitHub
#777 (comment)
.

@Alex-D
Copy link
Contributor

Alex-D commented Jun 5, 2014

Ouais enfin là t'as plein de fichiers de changés, c'est un poil plus embêtant à relire (est-ce lié à gulp ou à bower ?). Je suis d'accord, ça reste simple, c'est juste que @sandhose va devenir un contributeur régulier je pense, et donc c'est mieux qu'il nous fasse tout bien pour la suite :)

@sandhose
Copy link
Contributor Author

sandhose commented Jun 5, 2014

Alors...

D'abords, j'avoue avoir fait cette PR un peu à l'arrache, je devais partir... Je m'attendais pas à ce qu'elle soit merge tout de suite, et je l'ai envoyé justement pour avoir un retour ;)

L'explication d'installation, je vais la faire soit dans le README, soit dans un nouveau fichier dans doc/, avec bien évidemment un lien dans le README.

Pour ce qui est de rassembler vendors & custom, je suis pas tout à fait d'accords... Si c'est par rapport aux nombre de requêtes, c'est pas une requête de plus qui va changer grands chose :p
De plus, le vendors.js est moins susceptible de changer (on va pas rajouter des dépendances toutes les 5min ^^ ), et sera facilement cachable pour le navigateur...

Pour les JS séparé pour le dev, il y a possibilité je pense de générer des sourceMaps (je vais essayer me documenter la dessus), pour aider au développement. Après, copier les fichiers JS bruts dans le dossier dist est également possible pour le développement

Le fait d'avoir mis tout dans un dossier main/ viens du fait que je ne savais pas où caser le newsletter.js... J'aurais pu tout mettre dans un dossier, et dire dans le fichier de config de l'exclure du concat, mais j'ai préféré le séparer du reste

Le watch est prévu, ainsi que LiveReload, je n'ai juste pas eu le temps de mettre ça correctement en place 😄

Enfin, je suis d'accords avec @geoffreyc , pour moi Gulp et Bower vont ensemble, d'où le fait qu'il n'y a qu'une seule PR ;)

@sandhose
Copy link
Contributor Author

sandhose commented Jun 5, 2014

@Alex-D Les 53 fichiers modifiés sont surtout dus au dossier assets/css/scss/ qui a été déplacé vers assets/scss/... En soit, bower, c'est 2 fichiers en plus, et les dépendances JS virés du répo ;)

@Alex-D
Copy link
Contributor

Alex-D commented Jun 5, 2014

Si c'est par rapport aux nombre de requêtes, c'est pas une requête de plus qui va changer grands chose :p

Si, crois moi. On fait les choses bien ou on ne les fait pas :p Donc là, on les fait bien :)

et sera facilement cachable pour le navigateur...

Le cache expire sera le même sur les deux fichiers, ça ne change pas grand chose.

je ne savais pas où caser le newsletter.js

Ah oui, ok. J'ai envie de dire, c'est plutôt lui qu'il faut déplacer, cette page va virer à la sortie, donc privilégions le confort de dev à long terme :)

En soit, bower, c'est 2 fichiers en plus, et les dépendances JS virés du répo ;)

C'est plus pour l'avenir que pour là que je disais ça :)

@sandhose
Copy link
Contributor Author

sandhose commented Jun 5, 2014

Quoi qu'il en soit, je vais finir tout ça de mon côté, et j'envoie tout demain soir, avec le watch, livereload et tout et tout ;)

@Alex-D
Copy link
Contributor

Alex-D commented Jun 5, 2014

Tu peux commit au fur et à mesure, comme ça je fais une review progressive :)

Merci d'avance en tous cas !

@Alex-D Alex-D added the pr:front label Jun 5, 2014
@sandhose
Copy link
Contributor Author

sandhose commented Jun 6, 2014

Le gulp watch est mis en place, j'ai juste quelques problèmes avec LiveReload, qui marche pas avec Compass... Du coup, je l'ai pas encore inclus, et je verrais ça se soir

J'ai crée un dossier assets/misc/, où les ressources sont purement et simplement copiés (utiles donc pour les newslettes.{js,css}), et j'en ai donc profité pour mettre tout le JS dans assets/js/, et plus assets/js/main/.

J'ai aussi rapidement rajouté la génération d'un fichier js/all.min.js, qui comprends JS Vendors + Custom en 1 fichier minimifié.

Pour résumer: plus que le README, LiveReload, et JSHint ;)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 11a534e on sandhose:feature-771-gulp into 0a1a360 on zestedesavoir:beta-0.2.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling fcb72cd on sandhose:feature-771-gulp into 0a1a360 on zestedesavoir:beta-0.2.

@sandhose sandhose mentioned this pull request Jun 6, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 0601e81 on sandhose:feature-771-gulp into 0a1a360 on zestedesavoir:beta-0.2.

@Taluu
Copy link
Contributor

Taluu commented Jun 6, 2014

Je pense aussi que les deux vont ensemble.

Ceci dit, un petit squash des commits serait bien sympa (pour les commits de tests travis :-°) ! :)

@@ -3,6 +3,8 @@ git:
language: python
python:
- "2.7"
cache:
- apt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je ne sais pas si tu essaye de faire du cache là, mais si j'en crois la doc de travis :

The features described here are currently only available for private repositories on travis-ci.com

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok j'avais mal lu la doc, je pensais accélérer le build travis au passage (parce que 15min la build, c'est long à attendre, surtout quand tu sais pas si ça va marcher!)

@sandhose
Copy link
Contributor Author

sandhose commented Jun 6, 2014

Désolé pour les commits travis! 😕 (C'est possible de faire un squash, sans balancer sur une autre branche, et refaire une PR ? 😒 )

Sinon, j'ai documenté l'installation et l'utilisation des différents outils! 😄
Alors c'est sûrement pas la meilleure doc possible, mais il y a l'essentiel, à savoir l'installation sur Windows/OS X/Linux, et la description des différentes tâches gulp

J'ai aussi adapté les fichiers à JSHint de mon côté en local, et j'attends juste que cette PR soit merge pour pouvoir envoyer ça... (cf. #782 )

@@ -55,7 +61,10 @@ Comment démarrer une instance de ZdS ?
- Cloner le dépot git *via la console git* (et pas via powershell) windows: `git clone https://github.com/Taluu/ZesteDeSavoir.git`
- Dans la console PowerShell via l'environnement zdsenv installez les dépendances.
- `easy_install lxml`
- `npm install -g bower gulp`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je crois qu'il manque ici la procédure d'installation de npm sous windows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPM s'installe avec NodeJS...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mais effectivement, j'ai pas mis dans le README l'install de Node pour Windows! Je vais rajouter ça tout de suite, merci!

@Taluu
Copy link
Contributor

Taluu commented Jun 6, 2014

Yes, suffit de faire un rebase interactif sur ta propre branche. Par exemple, ici c'est git rebase -i upstream/beta-0.2. Tu pourras manipuler les commits comme tu le souhaites (en remplaçant upstream par l'origine de ta branche) :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 09b747f on sandhose:feature-771-gulp into 0a1a360 on zestedesavoir:beta-0.2.

@firm1
Copy link
Contributor

firm1 commented Jun 6, 2014

Je préfère tester personellement tout ça (surtout le déploiement sur le serveur) avant de merger la PR.

Du coup je ne comprend pas pourquoi on garde django-pipeline si les fichiers statics sont déjà packagés

@sandhose
Copy link
Contributor Author

sandhose commented Jun 6, 2014

Je ne sais pas comment django-pipeline fonctionne, donc je vais laisser quelqu'un d'autre s'occuper de le virer :)

@Taluu Impossible de re-push sur la même branche... Je suis vraiment obligé de rebaser et de push sur une nouvelle branche pour refaire une PR ? :c

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 09b747f on sandhose:feature-771-gulp into 0a1a360 on zestedesavoir:beta-0.2.

@Taluu
Copy link
Contributor

Taluu commented Jun 6, 2014

git push -f origin ta-branche :)

(Dans ce cas, même si réécrire l'historique en git est bof, comme c'est ta branche, personne ne devrait se baser dessus à priori, donc tu peux te le permettre)

@sandhose
Copy link
Contributor Author

sandhose commented Jun 6, 2014

Voila c'est bon, merci!

Du coup, j'appelle à tester de votre côté, pour voir si mes instructions sont cohérentes, et si le build gulp marche bien chez vous!

@firm1
Copy link
Contributor

firm1 commented Jun 7, 2014

ça m'a l'air de fonctionner comme il faut (en tout cas sur mes OS de test windows et ubuntu).

Maintenant, en ce qui concerne le projet en lui même. je me demande si ça ne vaut pas le coup de faire rentrer le dossier dist dans le repo. Pourquoi ?

  • un developpeurs fait rarement le back-end et le front-end en même temps. Donc pour un dev back, si on peut lui éviter d'installer tout la machine de front (compass, node, gulp, etc.) juste pour faire ses tests sur back, ça peut être pas mal, l'inverse aussi est vrai.
  • je compte réorganiser le readme pour avoir d'une part, une install pour le back et les élements optionnels qui ne servent qu'au front-end.

Donc si tu rajoute le dist (qui fait 1.5Mo) chez moi dans la PR, je merge le bébé.

@sandhose
Copy link
Contributor Author

sandhose commented Jun 7, 2014

Tu es sûr ? Parce que pour moi l'un des intérêts de la chose était justement de ne pas avoir ces fichiers dans le répo, et donc de ne pas mettre à jour tout ce qui est CSS compilé, Sprite (qui change de nom à chaque compile) dans des commits, alors que ce n'est pas l'objet

On peut cependant garder dans le .gitignore le dossier dist/, mais tout de même inclure une version du dossier dist/, que l'on met à jour régulièrement, quand il y a eu de grosses modifications dans le front...

Cela permet d'avoir un truc qui marche direct pour ceux qui ont la flemme d'installer quelques outils, et en même temps de pas polluer les commits avec des modifications de fichiers compilés... (en plus, les diffs de ces fichiers est souvent lourds...)

[troll]En plus, je vois pas pourquoi nous au front on se ferait chier à installer python, les dépendances pip et tout le bordel, alors qu'au back ils auraient pas besoin d'installer les outils pour le front[/troll]

@firm1
Copy link
Contributor

firm1 commented Jun 7, 2014

On peut cependant garder dans le .gitignore le dossier dist/, mais tout de même inclure une version du dossier dist/, que l'on met à jour régulièrement, quand il y a eu de grosses modifications dans le front...

Hmm, pour le coup, je ne pense pas qu'il faut rajouter de la complexité dans notre façon de pusher sur le repo (je le trouve déjà assez complexe ainsi).
Personnellement, je ne trouve pas forcément que la MAJ des sprite (qui n'arrive pas tous les jours) est si polluant que ça. Mais quand je pense a tout ce qu'il y'a a installer pour faire tourner une instance de zds minimale, j'aimerai autant que possible limiter les outils.

Après, c'est mon point de vue, faudrait peut-être des avis.

@sandhose
Copy link
Contributor Author

sandhose commented Jun 7, 2014

Hmm, pour le coup, je ne pense pas qu'il faut rajouter de la complexité dans notre façon de pusher sur le repo (je le trouve déjà assez complexe ainsi).

git add --force dist/ :-°

Et puis on a pas besoin de faire ça toutes les 5 minutes! Suffit de faire ça une fois tous les ~2 jours, et ça ira! Les devs back auront une copie suffisamment à jour pour pouvoir bosser, sans installer ces outils... (et puis si quelqu'un prévoit de participer activement au développement, autant qu'il ai tous les outils, qui sont vites installés :-° )

De plus, je pense que si on est plusieurs à bosser sur le front, ça risque surtout de créer des conflits inutiles sur main.min.js, main.min.css, ou autre...

@firm1
Copy link
Contributor

firm1 commented Jun 7, 2014

De plus, je pense que si on est plusieurs à bosser sur le front, ça risque surtout de créer des conflits inutiles sur main.min.js, main.min.css, ou autre...

pas faux.

Du coup, la solution de refresh du dist de temps en temps me va. Tu pousse l'init ?

@sandhose
Copy link
Contributor Author

sandhose commented Jun 7, 2014

En plus, je trouve que pour un petit changement comme ça, il devrais pas y avoir un diff comme ça :-°

@sandhose
Copy link
Contributor Author

sandhose commented Jun 7, 2014

Well, c'est peut-être pas la solution, puisque si dist/ est vide (gulp clean), il affiche que tous les fichiers dans dist/ ont été supprimés dans git status... De même si un fichier a été modifié... J'annule la commit

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d4e81a7 on sandhose:feature-771-gulp into 63c5dc9 on zestedesavoir:beta-0.2.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d4e81a7 on sandhose:feature-771-gulp into 63c5dc9 on zestedesavoir:beta-0.2.

@Taluu
Copy link
Contributor

Taluu commented Jun 7, 2014

Honnetement, je ne pense aps qu'il faut inclure le dist dans le répo. Sinon, on est bien parti pour inclure les dépendances installées par pip (parce que "les devs front s'en foutent presque des dépendances back", si je reprend l'argument avancé)

@Taluu
Copy link
Contributor

Taluu commented Jun 7, 2014

Surtout si le dépot est suffisement lourd comme ça (cf les remarques y'a qqs temps de @SpaceFox et @Alex-D)

@sandhose
Copy link
Contributor Author

sandhose commented Jun 7, 2014

Au pire, on peut host quelque part une archive (tar, zip, ou whatever) contenant tout le dist/ (qui peut être généré via une tâche gulp), si vraiment y'en a qui veulent pas ou qui n'arrivent pas à installer les dépendances front.
Mais sinon je suis du même avis que @Taluu

@firm1
Copy link
Contributor

firm1 commented Jun 7, 2014

Let's go

firm1 pushed a commit that referenced this pull request Jun 7, 2014
Intégration de Gulp et de bower (#771)
@firm1 firm1 merged commit 4a25fee into zestedesavoir:beta-0.2 Jun 7, 2014
@sandhose sandhose deleted the feature-771-gulp branch June 7, 2014 21:46
@SpaceFox
Copy link
Contributor

SpaceFox commented Jun 8, 2014

Je pinge sur cette discussion pour dire que j'ai ouvert un topic sur l'état
de l'environnement de développement suite à ce merge :
http://zestedesavoir.com/forums/sujet/237/lenvironnement-de-developpement/

2014-06-07 23:41 GMT+02:00 firm1 notifications@github.com:

Merged #777 #777.


Reply to this email directly or view it on GitHub
#777 (comment).

@sandhose
Copy link
Contributor Author

sandhose commented Jun 8, 2014

(Au passage, j'ai pas accès à la pré-prod, moi :-°)


Sent from Mailbox for iPhone

On Sun, Jun 8, 2014 at 3:57 PM, SpaceFox notifications@github.com wrote:

Je pinge sur cette discussion pour dire que j'ai ouvert un topic sur l'état
de l'environnement de développement suite à ce merge :
http://zestedesavoir.com/forums/sujet/237/lenvironnement-de-developpement/
2014-06-07 23:41 GMT+02:00 firm1 notifications@github.com:

Merged #777 #777.


Reply to this email directly or view it on GitHub
#777 (comment).


Reply to this email directly or view it on GitHub:
#777 (comment)

@firm1
Copy link
Contributor

firm1 commented Jun 8, 2014


Sent from Mailbox for iPhone

Avec une signature pareille, ça va pas s'arranger :)

Je te contacte en MP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants