-
Notifications
You must be signed in to change notification settings - Fork 541
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
Replace image tags with picture tags #465
Conversation
@@ -1,6 +1,6 @@ | |||
{% load wagtailimages_tags %} | |||
|
|||
<figure> | |||
{% image self.image fill-600x338 loading="lazy" %} | |||
{% picture self.image format-{avif,webp,jpeg} fill-{400x220,600x338} sizes="(max-width: 768px) 200px, 900px" loading="lazy" %} |
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 chose 200px here for the mobile 'size' because of pixel density. When testing, if I made this number larger, the browser would choose the larger (600x338) image instead when emulating a mobile.
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 left small nitpicks/questions but the code changes look good to me.
@@ -202,6 +202,8 @@ | |||
("ar", "العربيّة"), | |||
] | |||
|
|||
WAGTAILIMAGES_AVIF_QUALITY = 60 |
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.
Why are we changing this?
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 played around with this number a bit to get .avif images that are smaller in size and still look good. If this value isn't explicitly set, the .avif renditions are larger than their jpg equivalents
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 see, that makes sense. Thanks!
- Uses Wagtail 5.2 picture tags https://docs.wagtail.org/en/latest/releases/5.2.html#responsive-multi-format-images-with-the-picture-tag - Each image now has three renditions for avif, webp and jpeg formats - Some images now have two sizes on top of this (6 renditions in total) for different screen sizes
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.
Thanks @rachelhsmith & @Tijani-Dia
I have rebased this and will merge in shortly.
I have moved the removal of the unused header include template to a standalone commit and remove the change to pre-commit config (that can be a new PR if anyone is up for it).
.pre-commit-config.yaml
Outdated
@@ -10,7 +10,7 @@ repos: | |||
args: ['--target-version', 'py37'] | |||
- repo: https://github.com/timothycrosley/isort | |||
# isort config is in setup.cfg | |||
rev: 5.6.4 | |||
rev: 5.12.0 |
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 this be done as a separate PR, just avoids noise on this PR.
I will remove this change when rebasing.
e2cb93c
to
bb23685
Compare
This PR replaces all 'image' tags with Wagtail 5.2 'picture' tags to reduce the weight of images.