-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: Webp images with fallback #99
base: master
Are you sure you want to change the base?
Conversation
_includes/footer.html
Outdated
</ul> | ||
<a target="_blank" href="mailto:[email protected]">[email protected]</a> | ||
</div> | ||
<div class="col-md-6 text-center"> | ||
{% asset logo-small.png alt="PyCon India 2019 Logo" width="70%" %} | ||
<picture> |
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 am not very comfortable with this either.
Ideally, we should have a custom tag(say image
instead of srcset
) and it should be invoked like
{% image src=logo-small.png alt="PyCon India Logo" %}
and that should render
<picture>
<source srcset="./assets/images/logo-small.png" type="image/png>
<source srcset="./assets/images/logo-small.webp" type="image/webp">
</picture>
Like we discussed earlier, if generating webp images is the issue, we can offload it from this PR. Let's manually check-in webp equivalents with their checksum. I don't really think we are going to keep adding images from now. That way, the only thing that needs to be done is implementing this custom tag. I can help with manually generating webp images if you want.
Let me know what you think.
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.
Manually generating webp isn't quite the problem I am facing. I've update PR, need a little help with the checksum part though
tag = "<picture> | ||
<source srcset='./assets/images/#{@args[:src]}.webp' type='image/webp'> | ||
<img src='./assets/images/#{@args[:src]}.png' alt='#{@args[:alt] unless @args[:alt].nil?}' #{width unless @args[:width].nil?}> | ||
</picture> |
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.
Rather than checking for each attribute like this, can we just loop over all attributes and add them here?
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.
@astronomersiva Can you check now ?
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.
this looks good...I believe the only issue now is with versioning the files(the checksum)?
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.
@astronomersiva Yes. Do you have any suggestions wrt versioning ?
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 you try using the jekyll assets helper here? Let's try that out and see
Not quite the solution I like, but need a feedback, if this working out in the right direction
Fix #45