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

Remplacement de Gravatar par Jdenticon pour les avatars par défaut #6609

Merged
merged 3 commits into from
Oct 5, 2024

Conversation

Situphen
Copy link
Member

PR liée au sujet sur le forum concernant l'intégration des sources externes

J'ai modifié les templates car Jdenticon utilise des éléments <canvas> ou <svg> au lieu de <img>. On peut utiliser cet outil pour personnaliser les avatars générés si on veut. Personnellement je trouve que les paramètres par défaut sont déjà très corrects !

Seul bémol avec ce remplacement : les membres actuels qui utilisaient Gravatar pour afficher leur avatar auront un avatar par défaut de Jdenticon. Ce que l'on peut faire pour éviter ça, c'est un script qui fait une requête vers Gravatar et si la réponse retourne un code 200 alors on ajoute l'URL de Gravatar en tant qu'avatar. Cela donnerait quelque chose comme ceci :

hash = md5(user.email.lower().encode("utf-8")).hexdigest()
gravatar_url = f"https://secure.gravatar.com/avatar/{hash}?d=404"
# Requête vers gravatar_url
# Si réponse 200
profile.avatar_url = gravatar_url
profile.save()
# Sinon, rien

QA :

  • source zdsenv/bin/activate && make update && make zmd-start && make run-back
  • Vérifier que les avatars s'affichent correctement partout sur le site (avatars par défaut ou personnalisés)

@coveralls
Copy link

coveralls commented Apr 21, 2024

Coverage Status

coverage: 89.182% (-0.003%) from 89.185%
when pulling 625c3f2 on Situphen:gravatar
into 92a7653 on zestedesavoir:dev.

@philippemilink
Copy link
Member

Ce que l'on peut faire pour éviter ça, c'est un script qui fait une requête vers Gravatar et si la réponse retourne un code 200 alors on ajoute l'URL de Gravatar en tant qu'avatar.

En réunion de dev's, on a confirmé que c'est ce qu'on voulait faire, a priori dans une migration.

@Situphen Situphen force-pushed the gravatar branch 4 times, most recently from 89c492a to 9f018d9 Compare September 28, 2024 23:12
@Situphen Situphen marked this pull request as ready for review September 28, 2024 23:12
@Situphen
Copy link
Member Author

C'est prêt pour la QA ! J'ai ajouté une commande pour la migration des membres avec un Gravatar.

zds/member/models.py Outdated Show resolved Hide resolved
Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

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

J'étais à deux doigts de déployer sur la bêta pour tester la commande de migration, je me suis dit que j'allais quand même tester en local... et j'ai bien fait :)

Il y a problème d'affichage des avatars des posts d'un sujet :
Capture d’écran_2024-10-02_22-52-22

J'ai corrigé le problème en remplaçant cette ligne

par

.avatar {

Ça corrige le problème, même si le membre est banni (le bandeau s'affiche bien en-dessous).

Les autres affichages possibles des avatars semblent corrects.

(et désolé de faire la QA au compte-goutte... :( )

@Situphen
Copy link
Member Author

Situphen commented Oct 2, 2024

Bien vu, c'est corrigé !

@Situphen
Copy link
Member Author

Situphen commented Oct 5, 2024

C'est modifié !

@philippemilink philippemilink merged commit 77c115d into zestedesavoir:dev Oct 5, 2024
12 checks passed
@Situphen Situphen deleted the gravatar branch October 5, 2024 19:42
Copy link
Member

@AmauryCarrade AmauryCarrade 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 vois cette PR qu'une fois fusionnée mais je me permets quand même de laisser un commentaire. Un peu tard pour cette PR mais peut-être pour une autre :) .


{# Template used by the templatetag "avatar" defined in zds/utils/templatetags/profile.py #}

{% captureas alt_text %}Avatar de {{ username }}{% endcaptureas %}
Copy link
Member

Choose a reason for hiding this comment

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

Dans la majorité des cas, il ne faudrait pas de texte alternatif ici. En effet, l’avatar est la plupart du temps affiché à côté du nom du membre concerné. Il est dans un tel cas purement décoratif et l'information donnée est en doublon (lecture similaire à “Avatar de Situphen. Image. Situphen.”). Ce n'est pas pour rien que dans l'ancienne version du code, l'attribut alt était vide : c'était une bonne pratique.

La seule alternative serait de permettre aux membres de fournir un texte alternatif pour leur avatar, mais là encore — ce ne serait pas pertinent partout (probablement juste sur leur page de profil ; sinon, ce serait très redondant sur les flux de discussion à force d'être répété à chaque message… ou éventuellement, juste au premier rencontré… et encore).

<img src="{% avatar_url profile %}" alt="Mon compte" class="avatar">
{% avatar profile %}
Copy link
Member

Choose a reason for hiding this comment

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

Ici le texte alternatif peut être pertinent (bien que ce ne soit pas la meilleure manière de faire passer cette information dans ce contexte), car l'image est en réalité un lien vers une page spécifique ; or, il a été supprimé, ce qui fait qu'on ne peut plus savoir, avec des outils d'assistance ou des robots d'exploration, vers quoi pointe cette page.

Une solution propre serait de rendre l'avatar décoratif (texte alternatif vide) et d'ajouter le nom du lien masqué (.visuallyhidden, chez nous, souvent appelé .sr-only ailleurs) ; ou de renommer entièrement le lien englobant avec un aria-label, éventuellement.

Par exemple, quelque chose de cette idée.

{% avatar profile %}
<span class="visuallyhidden">Mon compte</span>
<span class="username label">{{ user.username }}</span>

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

Successfully merging this pull request may close these issues.

4 participants