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

[responsive()]: Step is not taken into account #93

Open
fcisio opened this issue Aug 30, 2021 · 12 comments
Open

[responsive()]: Step is not taken into account #93

fcisio opened this issue Aug 30, 2021 · 12 comments
Labels
enhancement New feature or request

Comments

@fcisio
Copy link

fcisio commented Aug 30, 2021

Hi, I'm using the React SDK and the responsive() plugin doesn't seem to work properly for me.

Basically, regardless of the value I input in the plugin (ex: 400, [400, 800, 1200]), the scale that is fetched always matches the parent element.

So I get weird scales like 512, 647, ...

What I would expect

  • If the step is set to 400, the only scales that can be fetched would be 400, 800, 1200, 1600, ...
  • On retina, if the container width is 400, the image fetched should be 800
  • Images set to object-fit: cover, should still look good. (The aspect ratio of the image should also be taken into account when comparing to the container)

The SDK V1 was more complete regarding the responsive behaviors.

Alternatives

  • Add a sizes prop (setting it would opt-out of the container width; based responsive)
@fcisio
Copy link
Author

fcisio commented Aug 30, 2021

For the time being, I've created my own plugin based on the original one.
The gist of it is: element.parentElement.clientWidth * window.devicePixelRatio

requires a bunch more code for good enough support + listen to pixel density change (along with resize)

@momoip
Copy link

momoip commented Sep 7, 2021

Hi fcisio,
I am checking with my teammates and will get back to you once I get some insights.

@fcisio
Copy link
Author

fcisio commented Sep 7, 2021

Thanks!
Here's a gist of my custom plugin if it can be useful.

It implements:

  • using the devicePixelRatio to serve appropriately sized images
  • using strict (step) sizing at initial to minimize infinite transformations (since it's based on the parent width)
  • allow specifying if the image width should be relative to his parent or the viewport (useful when using as background image cover) + the percentage of the viewport
  • min & max values that go along steps (useful to prevent scaling image up)

@momoip
Copy link

momoip commented Sep 7, 2021

Thanks fcisio.

@momoip
Copy link

momoip commented Sep 9, 2021

Hi fcisio,
We have created an internal ticket to investigate this issue.
Thanks,

@momoip
Copy link

momoip commented Sep 13, 2021

Hi fcisio,
Just to follow up with you that this is the expected behavior and we are looking into improving this feature in the future.
Thanks,

@KevinKreps
Copy link

How could this be expected behaviour? All documentation and guides say it works different. Also makes absolutely no sense regarding transformation billing. Is there any update on this?

@patrick-tolosa
Copy link
Contributor

@KevinKreps would you expect to get a stretched image if your container is larger than the image?

(Say the container is 1000px, but your responsive steps are [200,400,600] - would you expect to get a 600px image stretched to 1000?)

We believe that's undesired, however I'm curious to hear your opinion on this.

I'm also interested in the use case - in what situations would your responsive steps be so far from your containers?

Speaking of implementation details, we can consider a flag of sorts (pseudo code: useLastStepAsMaxWidth) but as I said, I'd love to hear more.

lastly, since this is indeed the expected behaviour (which we are now discussing here about changing), please share where does the documentation/guides state otherwise so we can adjust it.

@KevinKreps
Copy link

@patrick-tolosa Thanks for your superfast reply :)

If my container is 1000px and my steps go to 600px only, then yes, I would expect it to stretch resulting in shitty quality. I would also expect anyone working with steps to consider this case.

For example, if I use [200,400,600] I would make sure that my container sizes are all roughly in that range. If I want to have a more detailed view, I would conditionally not use the responsive plugin and manually set width and height (maybe even using original image). I could also define [200,400,600, 1000]. Then I would expect any container larger than 600 using the 1000px image. Also, I could do responsive(200). This will allow any step multiple of 200.

In my opinion, any one of these solutions is miles better than the actual behaviour right now, which is basically saying responsive(1). In this setup I would never use responsive as it could drive my transformation costs like crazy, rendering the plugin useless.

The idea with the flag is interesting. What would happen if it is set to false? Then fetch original?
Also interesting ideas to think about if not using the flag:

  • Mention in documentation and references that the last step is used as max width
  • give console warning if container exceeds max defined step, would help to find oopsies

e.g. this guide uses responsive(200)
https://cloudinary.com/documentation/responsive_images#automating_responsive_images_with_javascript_frontend_frameworks
Also, everywhere else the term "responsive" is used, it never states that it does not work with steps and even promotes using steps. Obviously includes other or older legacy plugins. Then still, why should we introduce inconsistency.

@patrick-tolosa
Copy link
Contributor

For example, if I use [200,400,600] I would make sure that my container sizes are all roughly in that range

That's what I meant when I mentioned "by design", the assumption the plugin makes is that people won't use steps that are far from their container width.
Given your statement, the plugin's behaviour is extremely similar to what you're describing (since as you said, your container is close to [200, 400, 600] anyway).

useLastStepAsMaxWidth can be used to create the outcome you described - if you provide the last step as 400, we will never fetch an image larger than 400.
if the flag is false we use the current behaviour - meaning the first image fetched will be based on the container.

Some use cases for clarity

  1. Container is 1000, steps are [200, 400], flag is enabled, -> image fetched is 400.
  2. Container is 1000, steps are [200, 400], flag is disabled(default), -> image fetched is 1000.

The OP also raised this as an expectation:

If the step is set to 400, the only scales that can be fetched would be 400, 800, 1200, 1600
that raises a different expectation - no image should be fetched out of those steps.

Aside from all that, the system was designed for extensibility - if the default behaviour is not suitable for you, you can create your own plugin as @fcisio did.
Our core plugins might be somewhat opinionated (as in this example).

Not sure what you mean here:

Also, everywhere else the term "responsive" is used, it never states that it does not work with steps and even promotes using steps.

Lastly, since you mentioned you a few times, can you please explain how this plugin could "drive your transformation costs like crazy"? - perhaps I'm missing something.

Please consider this is an opinionated discussion about the default behaviour, and we're considering all our customers - that's why we offer the option to customize it in the first place.

@KevinKreps
Copy link

useLastStepAsMaxWidth can be used to create the outcome you described - if you provide the last step as 400, we will never fetch an image larger than 400.
if the flag is false we use the current behaviour - meaning the first image fetched will be based on the container.

This would be a great solution!

Also, everywhere else the term "responsive" is used, it never states that it does not work with steps and even promotes using steps.

With this, I mean: Everywhere I was reading about the responsive plugin, steps were mentioned. I expected it to work as stated in the guides. Took me a little while to figure out that it was not implemented, and I didn't understand why it is not implemented.

Lastly, since you mentioned you a few times, can you please explain how this plugin could "drive your transformation costs like crazy"? - perhaps I'm missing something.

The reason of using steps is to reduce the variety of possible requests. Different/new transformation requests result in a transformation on cloudinarys side, which is billed. Moreover, the possible CDN cache hit decrease, resulting in longer loading for customers. This is a result of people using different browser sizes.
The result of the implementation now is, that at the end of the day I will have nearly every possible image size transformed and cached, but I don't see this as a feasible goal, as I would have to squander my transformations going down this path.

I really hope this will get implemented soon, if not it will be necessary to look for other solutions as you stated. Could you please keep us up to date regarding this topic and any final decision?

@patrick-tolosa
Copy link
Contributor

Thanks @KevinKreps - it's much clearer now!
We will of course update on any progress here, as well if we decide to leave this be.

I get where you're coming from regarding transformation costs - something to keep in mind moving forward.

I'll provide an update when I have more information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants