-
Notifications
You must be signed in to change notification settings - Fork 16
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: display correct image for codebases in ogp tags #681
Conversation
django/library/jinja2/library/codebases/releases/retrieve.jinja
Outdated
Show resolved
Hide resolved
also just realized the tags are duplicated across each page, using a macro https://jinja.palletsprojects.com/en/3.1.x/templates/#macros with the relevant params (title, description, optional image url) would cut down on quite a bit of markup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks Charles. 2 nitpicks and then this is good to go
<!-- Twitter Meta Tags --> | ||
<meta name="twitter:card" content="summary_large_image"> | ||
<meta property="twitter:domain" content="comses.net"> | ||
<meta property="twitter:url" content="{{ request.build_absolute_uri(absolute_url) }}"> | ||
<meta name="twitter:title" content="{{ codebase.title }}"> | ||
<meta name="twitter:description" content="{{ codebase.summarized_description }}"> | ||
<meta name="twitter:image" content="{{ codebase.get_featured_image() }}"> | ||
{% if codebase.featured_images.exists() %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can combine this if/else block with the prior to avoid duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still the case, especially with a macro. Probably best to just modify get_featured_rendition_url() to return None if the featured image doesn't exist and then we'd just need one line in the template:
{{ ogp_tags(image=codebase.get_featured_rendition_url(), ...) }}
django/core/jinja2/base.jinja
Outdated
<meta name="twitter:title" content="{{ title or 'The Network for Computational Modeling in the Social and Ecological Sciences (CoMSES Net)'}}"> | ||
<meta name="twitter:description" content="{{ description or 'CoMSES Net is an international open research community dedicated to fostering good practices for computational / agent based modeling.'}}"> | ||
<meta property="og:image" content="{{ image or static('images/logo-comses.png') }}"> | ||
{% endmacro %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go in core/common.jinja with most of the other shared macros and then be explicitly imported where we use it
Co-authored-by: Scott Foster <[email protected]> Co-authored-by: Aiko Muraishi <[email protected]> Co-authored-by: Charles Sheelam <[email protected]>
remove duplicated `og:image` and use OGP tags as described in https://developer.twitter.com/en/docs/twitter-for-websites/cards/guides/getting-started
Fixed the image tags in
django/library/jinja2/library/codebases/releases/retrieve.jinja
to display the correct image.Fixed image tag in
django/core/jinja2/core/jobs/retrieve.jinja
to the correct syntax.