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

Corrige les twitter cards sur les conteneurs #5521

Merged
merged 3 commits into from
Nov 18, 2019

Conversation

firm1
Copy link
Contributor

@firm1 firm1 commented Nov 14, 2019

Numéro du ticket concerné (optionnel) : #5447

Les tests ont été rajouté comme demandé dans le ticket.

Contrôle qualité

  • Lancez le site
  • Allez sur un contenu publié et vérifié que vous voyez bien dans les meta du html la propriété twitter:image pour un contenu
  • Allez sur un conteneur (chapitre, partie) publié et vérifiez que vous voyez bien dans les meta du html la propriété twitter:image correspondante à l'image du tutoriel et non à l'icone de zds.
  • Allez sur un conteneur (chapitre, partie) publié mais sans image et vérifiez que vous obtenez bien l'url de l'image du site.

@coveralls
Copy link

coveralls commented Nov 14, 2019

Coverage Status

Coverage decreased (-3.5%) to 73.925% when pulling 9d75768 on firm1:fix-5447 into 0624021 on zestedesavoir:dev.

@philippemilink
Copy link
Member

Tu peux corriger les erreurs de style que te reproche Travis avant que je fasse une review, stp ?

@philippemilink
Copy link
Member

Rapport de QA

  • article sans image:
    <meta property="twitter:image" content="http://zestedesavoir.com/static/images/article-illu.png">
  • billet sans image:
    <meta property="twitter:image" content="http://zestedesavoir.com/static/images/opinion-illu.png">
  • tutoriel sans image:
    <meta property="twitter:image" content="http://zestedesavoir.com/static/images/tutorial-illu.png">
  • partie d'un tutoriel sans image:
    <meta property="twitter:image" content="http://zestedesavoir.com/static/images/tutorial-illu.png">
  • chapitre d'un tutoriel sans image:
    <meta property="twitter:image" content="http://zestedesavoir.com/static/images/tutorial-illu.png">
  • tutoriel avec image:
    <meta property="twitter:image" content="http://zestedesavoir.com/media/galleries/23/634f4220-8b8d-44b0-821e-467eb03c85d4.png.144x144_q95_crop.png">
  • chapitre d'un tutoriel avec une image:
    <meta property="twitter:image" content="http://zestedesavoir.com/media/galleries/23/634f4220-8b8d-44b0-821e-467eb03c85d4.png.144x144_q95_crop.png">

Le bug à l'origine de l'issue est donc réglé.

Je n'ai pas fait tourner les tests, mais Travis semble content. Je n'ai pas compris tous les tests dont parlait @artragis dans son commentaire de l'issue, je le laisse regarder s'il est satisfait avec les tests qui ont été rajoutés.

J'attire l'attention sur le fait que en faisant tourner la version de dev en local, les URLs des meta qui pointent vers des images ont pour préfixe zestedesavoir.com. Il faudrait peut-être corriger ça ?

Suggestion: si le contenu n'a pas d'image, ce ne serait pas plus logique d'avoir comme image le logo de ZdS (plutôt que l'image par défaut du type de contenu) ?

@artragis
Copy link
Member

Les tests me semblent bon.

Pour le prefix, c'est géré par le fichier de conf logiquement. Je fais la revue asap comme ça on pourra merger.

@philippemilink
Copy link
Member

Pour le prefix, c'est géré par le fichier de conf logiquement.

Justement, je n'ai pas trouvé dans les fichiers de conf...

@artragis
Copy link
Member

TU utilises quelle conf?

@philippemilink
Copy link
Member

Généralement dev_fast, mais même en cherchant dans les autres fichiers du dossier settings, je ne rien trouvé qui pourrait dire à easy_thumbnail quelle URL de base utiliser pour générer les URLs.

@A-312
Copy link
Contributor

A-312 commented Nov 16, 2019

Je pensais que cette ligne :

'url': 'https://zestedesavoir.com',
était modifiée par :
ZDS_APP['site']['url'] = 'http://127.0.0.1:8000'

@artragis
Copy link
Member

c'est le cas, dans dev.py mais là le pb c'est qu'il a http://zestedesavoir pas https

@A-312
Copy link
Contributor

A-312 commented Nov 16, 2019

J'ai trouvé pourquoi :

{{ app.site.dns }}{% block meta_image %}{% spaceless %}
on utilise site.dns et non site.url

@firm1
Copy link
Contributor Author

firm1 commented Nov 18, 2019

Etant donné que la PR fait son job, et que le ticket des urls est traité dans la PR suivante , je pense qu'on peut merge celle-ci non ?

@artragis artragis merged commit aeb20f6 into zestedesavoir:dev Nov 18, 2019
@artragis
Copy link
Member

Comme promis, j'ai vérifier le point qui m'intéressait en review et du coup je merge.

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

5 participants