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

🐛 Image zoom level too low #336

Closed
mztiq opened this issue Dec 20, 2022 · 17 comments
Closed

🐛 Image zoom level too low #336

mztiq opened this issue Dec 20, 2022 · 17 comments

Comments

@mztiq
Copy link
Contributor

mztiq commented Dec 20, 2022

Describe the bug
The image zoom level on click is most of the time very low to the point it barely increases the size of the linked image.

To Reproduce
Steps to reproduce the behavior:

  1. Add an image (in my case 2536 x 1215 px) with the Figure Shortcode via ![Alt text](image.jpg "Image caption")
  2. Deploy the site and click on the linked image and notice the size barely increases.

Expected behavior
The image zoom should be bigger, so details can be seen too.

Screenshots
No Screenshot but see the first picture in this post as an example.

Desktop (please complete the following information):

  • OS: Windows 10 / Android 13
  • Browser: FireFox
  • Version 108.0.1

Hugo & Blowfish versions
Hugo: v0.106.0
Blowfish: v2.18.0

@nunocoracao
Copy link
Owner

will add a global param to disable the hugo pipelines that make the image smaller for perfomance. You can override and just use the original image... look for disableImageOptimization in the next release.

#339

@mztiq
Copy link
Contributor Author

mztiq commented Dec 27, 2022

I just tried out Blowfish v2.19.1 (coming from v2.18.0), seems like the feature images of posts in list view (layout = "hero") look kind of buggy.
Here is a before Screenshot:
grafik

And here is one after updating to V2.19.1:

grafik

Not entirely sure if this behaviour is related to the changes made regarding "Image zoom level".
Let me know if I should open up a new issue.

Edit: Looks like this issue exists since v2.19.0, v2.18.0 is working fine.

@nunocoracao
Copy link
Owner

@chrisbanes Is this related to #327?

@nunocoracao
Copy link
Owner

@mztiq Not related to the original ticket I think

@chrisbanes
Copy link
Contributor

Ah, that looks like Hugo's smart cropping. You can go back to the previous behaviour by changing the anchor value in your config.toml:

[imaging]
  anchor = 'Center'

@mztiq
Copy link
Contributor Author

mztiq commented Dec 27, 2022

This actually helped a lot, but the feature images are still slightly "zoomed in" (hard to notice if you don't have the screenshots side-by-side).

v2.18.0:

grafik

v2.19.1 with anchor = 'Center':
grafik

@nunocoracao
Copy link
Owner

Reopening so that I'll have a look at this one... @chrisbanes removing the change you added on #327 would provide the outcome @mztiq is looking for right? May I ask you to clarify the different between the two behaviours aka the one before your change and after the change? Thanks

@nunocoracao nunocoracao reopened this Dec 28, 2022
@chrisbanes
Copy link
Contributor

The difference is that the old behaviour resized the image to 600px always (maintaining the aspect ratio), then relied on CSS to cover the resulting box.

The new behaviour displays a pre-cropped image which has a roughly similar aspect ratio to what is displayed. It's not exact as the resulting div has a different size for different display sizes.

I don't personally think there's an issue here. An alternative approach would be to remove the crop, and resize based on a fixed height. That is a bit more wasteful though.

@nunocoracao
Copy link
Owner

@mztiq could you do a quick test by changing your local Blowfish theme files:

file: layouts/partials/article-link.html

Change
{{ with .Fill "600x400" }}
to
{{ with .Resize "600x" }}

And share screenshots. I want to check if this is the behavior you are going for.

@chrisbanes That might be an interesting approach. I was reading the image processing docs and need to understand better the .fill function. It seems that it does crop AND resize which might be causing the issue here. The resize option seems to be the one that always keeps the ratio. wondering if we should have a global var for this behavior across images or if that is overkill. thoughts?

@mztiq
Copy link
Contributor Author

mztiq commented Dec 28, 2022

@nunocoracao That's exactly the behaviour I'm going for, thanks for the suggestion.

Screenshot before:
grafik

Screenshot after changing to {{ with .Resize "600x" }}:
grafik

@nunocoracao
Copy link
Owner

@chrisbanes @mztiq my proposal:

Create a new global var in params.toml, imageProcessingMode with two values resize and fill. That way we can have both features in and people can chose which one applies to their use-case. Thoughts? If all agree I can move ahead with implementation for v2.20.0

@mztiq
Copy link
Contributor Author

mztiq commented Dec 29, 2022

@nunocoracao sounds great to me 👌.

@chrisbanes
Copy link
Contributor

My $0.02: this shouldn't be configurable. Adding config values for every possible combination is going to make this theme harder to maintain in the long term. It's your choice though.

@nunocoracao
Copy link
Owner

nunocoracao commented Dec 29, 2022

@chrisbanes that is a good point... already suffering a little for some similar past decisions.

3 options for voting (use emojis please):

❤️ Make it configurable - more maintenance work all options are covered now
🚀 keep current implementation fill - @mztiq would this work for you?
🎉 revert to previous implementation using resize - @chrisbanes would this work for you?

Tagging some other contributors to get extra opinions on this:
cc: @madoke @sergiod26 @Scoty @kszpieg @insidemordecai @lemonase @Coronon

@lemonase
Copy link
Contributor

@nunocoracao

Personally, I think keeping this at the partial layout/template level is a cleaner solution.

Adding a section in the docs that links to Hugo's image processing methods would also be helpful for those who want to customize.

@insidemordecai
Copy link
Contributor

Ah, that looks like Hugo's smart cropping. You can go back to the previous behaviour by changing the anchor value in your config.toml:

[imaging]
  anchor = 'Center'

Kinda agree with @lemonase, the only change to my config is adding @chrisbanes' suggestion so maybe linking/referencing Hugo's docs makes sense.

lemonase added a commit to lemonase/blowfish that referenced this issue Dec 29, 2022
Updated docs in reference to nunocoracao#336
@nunocoracao
Copy link
Owner

Thanks @mztiq @chrisbanes @lemonase @insidemordecai, already accepted the PR from @lemonase. Will close this comment and leave the current implementation as-is.

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

When branches are created from issues, their pull requests are automatically linked.

5 participants