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

Updated img syntax #27

Merged
merged 3 commits into from
May 18, 2018
Merged

Conversation

floede
Copy link
Contributor

@floede floede commented May 17, 2018

Using the syntax shown in Grav documentation:
https://learn.getgrav.org/content/media#cropresize-width-height

@floede
Copy link
Contributor Author

floede commented May 18, 2018

Oh I just noticed that the featured images still behave unexpectedly on the post.

For me at least they're not actually cropped, but just resized to the 4 by 3 format so they turn up skewed.

Is that the expected behaviour?

@floede
Copy link
Contributor Author

floede commented May 18, 2018

Further investigation reveals that cropZoom has the anticipated behaviour ie that the images are actually cropped.

Copy link
Owner

@robbinjohansson robbinjohansson left a comment

Choose a reason for hiding this comment

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

Thanks alot for investing your time into this PR, looks great! We should probably use cropZoom instead of cropResize as you mentioned.

@@ -18,7 +18,7 @@

{% if page.header.featured_image %}
<figure class="image is-4by3" style="margin-left: 0; margin-right: 0;">
<img src="{{ page.url }}/{{ page.header.featured_image }}?cropResize=768,576" alt="{{ page.title }}">
{{ page.media[page.header.featured_image].cropResize(768, 576).enableProgressive.html('',page.title|escape)}}
Copy link
Owner

@robbinjohansson robbinjohansson May 18, 2018

Choose a reason for hiding this comment

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

  1. Please change cropResize to cropZoom. As you mentioned in previous comment, cropZoom will resize and crop the image to ensure the resulting image is the exact size we want. More info on cropResize vs cropZoom.

  2. What does .enableProgressive do here? Can't find anything about it in docs. Isn't this usually enabled by default in /system/config/media.yaml? I'm not sure but I've got this in mine:

default:
  - enableProgressive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a guy in the Slack channel who suggested the .enableProgressive, but if it's hard to find documentation for and might even be redundant, I'm going to leave it out.

Copy link
Owner

@robbinjohansson robbinjohansson May 18, 2018

Choose a reason for hiding this comment

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

Sounds good! If it's needed in specific use cases maybe it's best to leave it up to people to add it on their own instead 👍

@@ -21,7 +21,7 @@
{% if post.header.featured_image %}
<div class="media-right">
<figure class="image">
<img src="{{ post.url }}/{{ post.header.featured_image }}?cropResize=160,120" alt="{{ post.title }}">
{{ post.media[post.header.featured_image].cropResize(160, 120).enableProgressive.html('',post.title|escape)}}
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think that cropResize works fine on the frontpage thumbnails.
Because it means that the images will just expand sideways if their format is too wide.
But maybe you can test that out, and see what you like?

Copy link
Owner

@robbinjohansson robbinjohansson May 18, 2018

Choose a reason for hiding this comment

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

Personally I'd prefer using cropZoom here as well, since cropResize always respects the aspect ratio of the image it's hard to actually control the image output in width and height.

For example if I want to have the current demo image in a portrait format, it wont be cropped as I expect:

{{ post.media[image.jpg].cropResize(120, 160).html()}} will result in the demo image being cropped to 120x90 which isn't really what I'd want (I'd expect it to be in a portrait format).

This would probably be an issue when dealing with portrait images rather than landscape as well.

Personally I'd like that image to be resized and cropped to the exact width and height I specified. Maybe it's just all about personal preference :)

Removed the .enablePRogressive setting as well
@@ -21,7 +21,7 @@
{% if post.header.featured_image %}
<div class="media-right">
<figure class="image">
{{ post.media[post.header.featured_image].cropResize(160, 120).enableProgressive.html('',post.title|escape)}}
{{ post.media[post.header.featured_image].cropZoom(160, 120).html('',post.title|escape)}}
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for being picky, but would you mind removing the extra/double indentation here before merging? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh that's no problem at all.
I'm used to following strict formatting rules at work :-)

But I'm not so used to work on php / twig.

@robbinjohansson robbinjohansson merged commit b76ebfc into robbinjohansson:develop May 18, 2018
@robbinjohansson
Copy link
Owner

robbinjohansson commented May 18, 2018

Great work @floede ! Merged, thanks! 🎉

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.

3 participants