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

WIP: Some little improvements #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 54 additions & 17 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
.DEFAULT_GOAL := start

.PHONY: start start_netlify build build_preview clean _common _build_common
.PHONY: start start-netlify netlify-build netlify-build-preview clean _common _netlify-build-common

# ==============================================================================
# Run Netlify in watch mode
Expand All @@ -12,7 +12,8 @@ start:
# -v, --verbose verbose output
yarn hugo server -s site -v

start_netlify:
# Target to test netlify-cms locally
Copy link
Contributor

Choose a reason for hiding this comment

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

[next time] le mot Target n'apporte pas d'info (on sait qui est dans un Makefile, donc qu'on est en face d'un taget). Cf Poorly Written Comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok merci pour l'info !

start-netlify:
make _common
netlify-cms-proxy-server &
sleep 3
Expand All @@ -23,16 +24,16 @@ start_netlify:
# ==============================================================================

# Target used by Netlify to build the prod version.
build:
make _build_common
netlify-build:
make _netlify-build-common
# -s, --source string filesystem path to read files relative from
# -d, --destination string filesystem path to write files to
# -v, --verbose verbose output
yarn hugo -s site -d ../dist -v

# Target used by Netlify to build his preview
build_preview:
make _build_common
netlify-build-preview:
make _netlify-build-common
# -s, --source string filesystem path to read files relative from
# -d, --destination string filesystem path to write files to
# -D, --buildDrafts include content marked as draft
Expand All @@ -41,28 +42,64 @@ build_preview:
yarn hugo -s site -d ../dist -D -F -v

clean:
# We should clean the repo, because Netlify does not clean the repo
# Options `-f -f` is not an error. See https://github.com/sogilis/Blog/pull/160#discussion_r422919097
# Following is advised by GitLab CI https://docs.gitlab.com/ee/ci/yaml/#git-clean-flags
# Remove untracked files (files referenced into `.gitignore` include),
# but we exclude `node_modules`.
# Following command is advised by GitLab CI, read:
Copy link
Contributor

Choose a reason for hiding this comment

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

[next time] Du coup, cela signifie qu'on sous-entend que clean n'est utilisé que par la CI. Ca ressemble a une dépendance logique du coup (l'implem de clean dépend de où clean est utilisé), ce qu'il faut éviter (cf Make logical dependencies physical).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pas forcément non.

Bon je vois qu'il faut que j'améliore cette PR.

# https://docs.gitlab.com/ee/ci/yaml/#git-clean-flags
git clean -f -f -d -x -e node_modules


# ==============================================================================
# Private target (used by other targets above, but no sens to use outside)
# ==============================================================================

_common:
make clean
yarn install --network-timeout 3000 --prefer-offline
yarn lint
yarn prettier:check
# reboot.css
rm -f ./site/themes/sogilis/assets/css/reboot.min.css
cp ./node_modules/bootstrap-reboot/dist/reboot.min.css* ./site/themes/sogilis/assets/css/
# netlifh-cms
cp ./node_modules/netlify-cms/dist/netlify-cms.js* ./site/static/admin/
## fonts
# fonts
rm -rf ./site/themes/sogilis/static/css/typeface-montserrat
cp -R ./node_modules/typeface-montserrat ./site/themes/sogilis/static/css/typeface-montserrat
rm -rf ./site/themes/sogilis/static/css/typeface-eb-garamond
cp -R ./node_modules/typeface-eb-garamond ./site/themes/sogilis/static/css/typeface-eb-garamond
# COPY netlify-cms
rm -f ./site/static/admin/netlify-cms.js*
cp ./node_modules/netlify-cms/dist/netlify-cms.js* ./site/static/admin/
# Remove Hugo temp files
rm -rf site/resources/
Copy link
Contributor

Choose a reason for hiding this comment

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

J'ai l'impression que ce rm fait un clean conceptuellement, non ?
Si oui, alors il devrait être dans le target clean je pense.

PS : idem peut-être pour les autres rm / cp


_build_common:
_netlify-build-common:
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui dit netlify-xxx, dit target exécuté par Netlify lors du "déploiement", non ?
Si oui, alors pourquoi faire tous ces checks vu qu'on contrôle complètement ce qui est fait lors du build ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parce que rien n'interdit d'essayer de déployer en local ;-) . Et ça coûte pas cher :-)

# TESTS to know if repo is clean for build
# =========================================
# Test if we have unstaged changes is not a necessity
# (`git clean` does not checkout them) but it's a good practice.
@if ! git diff --quiet --exit-code ; \
then \
>&2 echo -e "\n\n\\033[4;31mERROR:" \
"You have changes not staged for commit" \
"(see them thanks \`git status')." \
"Use \`git add' to add changes into the git stagging area," \
"or remove this changes.\\033[0m\n\n" ; \
exit 10 ; \
else \
echo -e '\nCool, you have no unstaged changes\n' ; \
fi
# Following test is needed otherwise `git clean` remove also untracked files not
# added into (.gitignore).
@if [[ "$$(git ls-files --others --exclude-standard | wc -l)" -gt 0 ]] ; \
Copy link
Contributor Author

@JulioJu JulioJu Sep 9, 2020

Choose a reason for hiding this comment

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

@jibidus je ne suis pas persuadé que ce soit cool qu'à chaque yarn start on doive être avec aucun untracked file et aucune modification non présente dans la stagging area.

Pour le build, ça me paraît être une bonne pratique.

Mais pour yarn start ça va juste être relou.

Non ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi on aurait besoin de supprimer des fichiers sur un yarn build ou yarn start d'ailleurs ?
Ca parait bien sournois, je ne pense pas qu'un dev s'attende a voir disparaître des fichiers sur ce genre de commande.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah mince .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Après le but c'est justement que y'ait pas de fichiers qui disparaissent sournoisement. C'est le cas à l'heure actuelle, c'est une erreur de ma part (désolé).

then \
>&2 echo -e "\n\n\\033[4;31mERROR:" \
"You have untracked files" \
"not added into '.gitignore' (see them thanks \`git status')." \
"Use \`git add' to add files into the git stagging area," \
"or remove this files.\\033[0m\n\n" ; \
exit 11 ; \
else \
echo -e "\nCool, you have no untracked files not included" \
"into '.gitignore'\n" ; \
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Cela me paraît bien compliqué tout ça (beaucoup de signes cabalistiques qui rendent la lecture difficile), il n'y a pas moyen de faire plus simple ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je crois que faire un git clean avant un build de mise en prod c'est une bonne pratique. GitLab CI fait comme ça.

Après pour des builds locaux y'a moyen de faire autrement oui.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je serais intétéressé de comprendre pourquoi tu trouves cela compliqué ?

C'est du Bash et du Makefile.

Copy link
Contributor

Choose a reason for hiding this comment

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

C'est compliqué pour moi parce que juste a la lecture, je ne peux pas voir rapidement s'il y a un problème (s'il manque un ", un \, un $ ou autre par exemple). Et je pense que c'est pareil pour toi qui en est pourtant l'auteur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah mince, pour moi ça va. D'autant plus que j'ai la coloration syntaxique ;-) .

Copy link
Contributor Author

@JulioJu JulioJu Sep 10, 2020

Choose a reason for hiding this comment

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

Et chez moi les \n et tout ils apparaissent en rouge quand j'écris et tout ;-) . Et les dollards donnent une autre colloration syntaxique.

Après j'ai bien testé.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je suis vraiment désolé de faire du code pas clair. Je te propose qu'on se téléphone un jour oû tu auras le temps pour essayer de voir ensemble comment procéder pour fixer le problème.

# We dont check if we have staged and not commited changes.
make clean
make _common
yarn lint
yarn prettier:check
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
"scripts": {
"start": "make start",
"start:netlify": "make start_netlify",
"build": "make build",
"build:preview": "make build_preview",
"build": "make netlify-build",
"build:preview": "make netlify-build-preview",
"prettier:format": "prettier --write \"{,site/**/}*.{json,ts,css,scss,yml}\"",
"prettier:check": "prettier --check \"{,site/**/}*.{json,ts,css,scss,yml}\"",
"lint": "eslint . --ext .js,.ts",
Expand Down
9 changes: 5 additions & 4 deletions site/themes/sogilis/assets/css/footer.css
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,11 @@ footer {
background-color: var(--light-electric-blue);
margin-left: -1.5em;

/* On boostrap, for body line-height: 1.5 */
/* We have 1.3 + 0.2 = 1.5 */
/* As it, the bar is on the baseline */
/* But as the font have a height, so strange to be on the baseline */
/* On boostrap, we have line-height: 1.5 */
/* Therefore if we want that the hyphen has 0.4 em of blanck space at bottom */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* Therefore if we want that the hyphen has 0.4 em of blanck space at bottom */
/* Therefore if we want that the hyphen has 0.4 em of blank space at bottom */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oupse ! Merci pour ta vigilence.

/* we must have a margin top of 0.9 em */
/* We have 1.3 + 0.2 === 1.5 */
/* And 1.3 === 0.9 + 0.4 */
height: 0.2em;
Copy link
Contributor

Choose a reason for hiding this comment

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

[Out of scope] On pourrait utiliser des variables pour exprimer une bonne partie de ce qui est en commentaire.
Pas sûr que ce soit possible en css, mais je pense que ça l'est en sass par exemple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bien sûr !

Oui y'a des variables en CSS. Je note d'améliorer ce point

margin-top: 0.9em;
}
Expand Down
2 changes: 2 additions & 0 deletions site/themes/sogilis/layouts/partials/head.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
content="{{ .Summary | default .Site.Params.SiteDescription }}"
/>

<!-- TODO generate only one CSS file with Webpack. Use also auto-prefixer -->

<link
rel="stylesheet"
href="{{ "css/typeface-eb-garamond/index.css" | relURL }}"
Expand Down