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

Propre #34

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Propre #34

wants to merge 47 commits into from

Conversation

tvroylandt
Copy link
Contributor

@tvroylandt tvroylandt commented Mar 14, 2021

Bonjour,

Cette PR a pour objet la fusion de la démarche PROPRE dans gouvdown. Elle permet notamment l'intégration de tout le travail fait dans le cadre de la construction de la brochure ici.

Pour résumer :

  • construction d'une fonction et d'un template spécifique
  • basculement pour la gestion des fonts et des logos sur ce qui se fait déjà dans gouvdown
  • refactoring d'une partie du CSS pour être plus général, par @julientaq
  • ajout d'une documentation spécifique au format vignette sur l'utilisation

La brochure originale a été basculée sous gouvdown ici : https://github.com/spyrales/propre.brochure

Ca ne couvrira pas tous les besoins (mais déjà certains), mais c'est un bon point de départ pour des forks vers des formats de publis spécifiques à certaines institutions.

Répond aussi à #25

@codecov
Copy link

codecov bot commented Mar 14, 2021

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d43ff2b) 60.92% compared to head (9740322) 64.12%.

❗ Current head 9740322 differs from pull request most recent head 7d023a5. Consider uploading reports for the commit 7d023a5 to get more accurate results

Files Patch % Lines
R/propre_brochure.R 92.85% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   60.92%   64.12%   +3.20%     
==========================================
  Files          10       11       +1     
  Lines         325      354      +29     
==========================================
+ Hits          198      227      +29     
  Misses        127      127              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@RLesur RLesur left a comment

Choose a reason for hiding this comment

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

Quelques remarques en complément de celles indiquées dans le code :

  • il n'y a pas de garantie que la feuille de style reset.css soit incorporée en premier, je trouve ça gênant
  • le fichier inst/resources/css/propre/modules/part.css est vide
  • pourquoi a-t-on un répertoire www à la racine du repos ? à mon avis, ce sont des scories qu'il faut retirer

number_sections = FALSE,
# self_contained = FALSE,
toc = FALSE,
css = c(css, book_css, width_css),
Copy link
Member

Choose a reason for hiding this comment

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

L'utilisation de l'argument css empêche de travailler avec self_contained = FALSE. Ce serait mieux de travailler avec des html_dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ca me semble être le point principal ici. Faut que je regarde comment faire (pas trop à l'aise avec les html_dependecies) et je m'en occupe derrière

Copy link
Member

Choose a reason for hiding this comment

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

Je peux m'en occuper, je l'ai fait des dizaines de fois

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Si ça te dérange pas je veux bien. Comme ça la prochaine fois je saurai. Merci =)

inst/resources/css/propre/book.css Outdated Show resolved Hide resolved
inst/resources/css/propre/interface/hack.css Outdated Show resolved Hide resolved
inst/resources/css/propre/modules/body.css Outdated Show resolved Hide resolved
inst/resources/css/propre/modules/fonts.css Outdated Show resolved Hide resolved
vignettes/use_propre.Rmd Outdated Show resolved Hide resolved
vignettes/use_propre.Rmd Outdated Show resolved Hide resolved
vignettes/use_propre.Rmd Outdated Show resolved Hide resolved
vignettes/use_propre.Rmd Outdated Show resolved Hide resolved
@tvroylandt
Copy link
Contributor Author

tvroylandt commented Mar 21, 2021

J'ai encore un aspect visuel étrange sur les dernières pages

image

Comme si les colonnes ne passaient pas bien. @julientaq pourras-tu jeter un coup d'oeil ?

Côté https://github.com/spyrales/propre.brochure ça a l'air de passer (j'ai juste un micro problème de positionnement de l'image).
image

Sinon à part ça, tout est nickel sur l'aspect

@julientaq
Copy link
Collaborator

J'ai encore un aspect visuel étrange sur les dernières pages

Je ne comprends pas d’où ça vient , j’ai pas de souci sur ma machine.
skeleton.pdf

du coup j’ai un doute sur la maj des paquets.

l’ordre c’est bien pull → install packages → knit, c’est ça?

ou est-ce que j’oublie quelque chose?

@tvroylandt
Copy link
Contributor Author

Comme vous pouvez le voir avec mes nombreux commits, je n'ai pas réussi à terminer la GHA rapidement (quoique 10 commits pour un truc presque fonctionnel je suis pas loin de mon record personnel). Je bloque sur :

Ca sort des artifacts ici avec les versions HTML et PDF : https://github.com/spyrales/gouvdown/actions

Dans les sorties Linux, MacOS et Windows, j'ai le même aspect que sur mon local (modulo le logo).

@tvroylandt
Copy link
Contributor Author

De mon côté, ça devrait être ok.

Copy link

@statnmap statnmap left a comment

Choose a reason for hiding this comment

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

Je ne sais pas quand vous avez l'intention de finaliser cet ajout.
Il me semble qu'un template {pagedown} serait intéressant dans le package.

#' @param ... Additional arguments passed to \code{bookdown::\link{html_document2}()}.
#' @param css A character vector of additionnal CSS file paths.
#' @param width_main_column Width of the main text column (in %)
#'
Copy link

Choose a reason for hiding this comment

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

Suggested change
#'
#' @param made_with Character. Add extra information to show how the document was made.
#'

Je pense qu'il manque cette documentation de fonction et le passage de roxygen2::roxygenise() pour que ce soit fonctionnel

jengelaere and others added 7 commits December 12, 2022 11:01
…'exécution des checks)

-- R CMD check results ---------------------------------------------------------------------------- gouvdown 0.0.0.9000 ----
Duration: 4m 0.3s

> checking installed package size ... NOTE
    installed size is  8.1Mb
    sub-directories of 1Mb or more:
      extdata     3.8Mb
      resources   3.6Mb

> checking for non-standard things in the check directory ... NOTE
  Found the following files/directories:
    'header.html' 'www'

0 errors v | 0 warnings v | 2 notes x
…te-de-région-centre-val-de-loire

-- R CMD check results ---------------------------------------------------------------------------- gouvdown 0.0.0.9000 ----
Duration: 4m 13.9s

> checking installed package size ... NOTE
    installed size is  8.1Mb
    sub-directories of 1Mb or more:
      extdata     3.8Mb
      resources   3.6Mb

> checking for non-standard things in the check directory ... NOTE
  Found the following files/directories:
    'header.html' 'www'

0 errors v | 0 warnings v | 2 notes x

R CMD check succeeded
Merge branch '46-maj-logo-préfète-de-région-centre-val-de-loire' into propre

# Conflicts:
#	README.Rmd
#	README.md
#	man/add_plot_header.Rd
#	tests/testthat/test-rmarkdown.R
@jengelaere
Copy link
Collaborator

J'ai testé la branche et ai essayé de la mettre à niveau des 2 autres créés récemment (logo Centre et Corse) mais l'argument made_with lorsqu'il comprend des caractères accentués (comme c'est le cas par défaut) fait planter la compilation du doc sur windows, enfin chez moi en tout cas.

@jengelaere
Copy link
Collaborator

Je proposerai bien de corriger l'argument made_with et d'indiquer que propre.brochure() est encore expérimentale (en raison des dernières difficultés évoqués en mars 2021 no résolues), mais de l'inclure dans le package sans plus tarder. Le résultat est déjà franchement top. C'est tout près chez moi.
@MaelTheuliere @RLesur quelque chose d'autre bloquait ? Il me semble, Romain, que tu étais allé au bout de ta review ?

@MaelTheuliere
Copy link
Collaborator

j'avais pas de réserve pour ma part

jengelaere and others added 3 commits November 17, 2023 14:04
1. supression des accents dans l'argument par défaut `made_with` pour que les checks passent sur windows
2. Ajout du badge expérimental à la fonction pour tenir compte de cette difficulté et des soucis de mise en page signalés par Thomas le 21 mars 2021
fix CI : suppression option mac pour l'installation des dépendances en echec
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.

6 participants