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

[FIX] Formatting volunteer certificate #582

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

Conversation

SnowViet
Copy link
Contributor

@SnowViet SnowViet commented Nov 7, 2022

Je suis pas sûr à 100% de la doc générée.

Voilà le résultat:
Attestation Benevole CAF Annecy.pdf

Copy link
Member

@jnguiot jnguiot left a comment

Choose a reason for hiding this comment

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

Merci pour le boulot, ca n'a pas dû être facile

def generate_template(template, user):
"""Generate an image containing the volunteer certificate.

:param template:
Copy link
Member

Choose a reason for hiding this comment

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

Il faudrait préciser la doc, notamment sur la description des paramètres.
Notamment, pourquoi passer le template en argument si la fonction est censé le générer? Et qu'appelle-t-on le template? L'image de fond du document? Je ne suis pas sur qu'on devrait appeler ça un template vu qu'il est déjà adapté à l'utilisateur.
Je propose donc de:

  • mettre cette ligne dans la fonction et donc de transformer le paramètre template en variable.
    template = Image.open(Configuration.VOLUNTEER_CERT_IMAGE)
  • De renommer la fonction en generate_skeleton() ou generate_background() ou équivalent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai renommé en generate_background(), par contre, si je met le template dans la fonction, je ne peux plus récupérer la largeur du template utilisé plus bas

Copy link
Member

Choose a reason for hiding this comment

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

Je pense que la largeur du template est égale à la largeur de l'image? Donc on pourrait surement le faire :)


# Start drawing main text
current_line_position = 0
ImageDraw.Draw(image).multiline_text(
Copy link
Member

Choose a reason for hiding this comment

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

Une amélioration possible: créer une fonction qui fait le draw et qui renvoie la longueur du text. Ca évite de répéter le texte dans deux paramètres de fonction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible, par contre, si je fait ça il faut faire passer tous les arguments dans la nouvelle fonction (police, spacing, position, etc).
Sinon je peux juste utiliser la variable "text", comme plus bas.

Copy link
Member

Choose a reason for hiding this comment

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

Oui, en effet, il faudrait passer tous les arguments. Ce n'est pas obligatoire.

@jnguiot
Copy link
Member

jnguiot commented Dec 29, 2022

@SnowViet Est-ce que tu as pu avancer un peu sur le sujet? Sinon, je fais les modifs rapidement et je merge :)

@SnowViet SnowViet force-pushed the formatting-volunteer-certificate branch from 26e9b82 to 2d0d915 Compare December 29, 2022 18:55
@SnowViet
Copy link
Contributor Author

@SnowViet Est-ce que tu as pu avancer un peu sur le sujet? Sinon, je fais les modifs rapidement et je merge :)

Je t'avoue que je n'ai pas du tout eu le temps de me pencher dessus. J'ai juste fait deux ou trois détails que je viens de push. Et j'aurais pas le temps de bosser dessus ces prochaines semaines malheureusement.

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.

2 participants