API: swap naturalSlideWidth and naturalSlideHeight for ratio

[mvasin]

Why would you specify two parameters that boil down to a single number anyway? It's a confusing redundancy.

Sure, it can help cut a line of code when using the library, but react docs mention that

We recommend naming props from the component’s own point of view rather than the context in which it is being used.

and it makes sense to me.

So if the CarouselProvider component doesn't care about the values, and only cares about the ratio between the values, let's call the prop ratio and deprecate naturalSlideWidth / naturalSlideHeight.

[tim-steele]

@mvasin I understand where you are coming from. We looked at it more from the point of view of people using the component. People generally know or have the size of the images that will be in the carousel.

We felt overall It is easier for a human to put those numbers in and do the math for them, then to have them figure out the ratio, including things like repeating, rounding, decimals, etc.

Overall, I don't think we see the benefit in making a major breaking change in removing the height and width in favor of just a ratio. Is there a problem you are encountering using height and width that ratio would solve?

[mrbinky3000]

I can understand why you think this might be an improvement but as someone who has had to support this project, you'd be amazed how hard it is to explain the concept of "aspect ratio" to some people. If I did change the name, I'd change it to aspectRatio and not ratio, because that is a more accurate and self-documenting variable name.

"Self documenting variable names" is computer programing concept that predates React. The natural width and natural variable names were chosen because that's what people see when they hover their mouse over an image in Chrome dev tool.

The hopes were that people would be able to grasp on to this easier than fractions converted to decimals. So while in some respects, I do agree with you, I say "why make people do the math manually" when the computer can do it for you, and far more accurately.

Problems with ratio:

• You're making people compute the ratio and some will get it wrong. They just will.
• They might not compute the ratio to the required amount of decimal places, which can lead to half-pixels on some browsers and throw all the math off. Example. 1.3 instead of 1.333333 (most people don't know Chrome goes to 6 decimal places in their math)
• The two variable names are self-documenting.

[mvasin]

When I started reading the readme, it looked to me that I'm given two values so I can specify something like <CarouselProvider naturalSlideWidth={300px} naturalSlideHeight={50vh} />, and readme text goes extra mile to clarify that this is wrong and all it needs is extract the aspect ratio (again, what if I have width/height in different measurement units? Another source for confusion, while ratio is always unitless).

aspectRatio is a perfect name.

@tim-steele The biggest trouble with these props for for me is the need to specify them in the first place, see #94. It doesn't relate really, but why not to clean up. I'm OK if you won't.

@mrbinky3000 My primary use case most of the time is using images of various resolutions with object-fit set to cover. So the word "natural" does not sound natural to me in this case - it falsely generalises something that is different for every instance at the lower level (natural dimensions of most of the images in my carousels are different).

Well, that's perfectly fine that your original use case does not mirror mine. I don't mind if you'll stick to what's already there, but why not to fix these three confusions:

1. Hmm, I have to provide both dimensions, so they should both matter
2. What units can I use for these dimensions?
3. natural... - ??

[tim-steele]

@mvasin Maybe our documentation/README is a little confusing and we could provide a better explanation as to what these are and why they matter, and even provide an additional call out just for the aspect ratio. Let us look into this a bit more.

[mvasin]

@tim-steele I exaggerate a bit, I don't say I actually had those questions, I mean these three points are the source of potential confusion that could be avoided with deprecating natural* and introducing aspectRatio instead. If you're not convinced, just close the issue.

[tim-steele]

@mvasin I think your point about it being a source of potential confusion is valid. I am not sure if aspectRatio is or isn't the answer. But I think it is something that we should review and find a solution for, because there are other developers in other issues that have experienced the same confusion. Let me keep this open.

Thanks for bringing this to our attention.

[EmilEriksen]

I'd like to add another use case that I feel is relevant to this discussion. I'm building a slider with picture elements which I use for art-direction. This means that the aspect ratio changes depending on the viewport size. Of course, it's possible to change naturalWidth/Height at the same breakpoints but that would probably be a bit cumbersome to implement and I'm not sure if there's even a general approach that would work for all of the many things you can do with the picture element. Also, it always feels a little iffy to mix styling with logic although I realize that's always particularly challenging with carousels. Ideally, I feel like the picture element should be a "first-class citizen" that's supported by a modern carousel. Again, I understand that that's probably quite challenging in reality.

Of course, this discussion also extends to the general case where the slides change aspect ratio depending on the viewport - especially when the slides contain content other than images. One common example, I'd imagine, is a testimonial slider. While you'd probably want each testimonial/slide to be the same height to avoid a janky/jumpy layout, you might want to change the aspect ratio on smaller screens due to the text being taller.

[asherccohen]

I'd like to add another use case that I feel is relevant to this discussion. I'm building a slider with picture elements which I use for art-direction. This means that the aspect ratio changes depending on the viewport size. Of course, it's possible to change naturalWidth/Height at the same breakpoints but that would probably be a bit cumbersome to implement and I'm not sure if there's even a general approach that would work for all of the many things you can do with the picture element. Also, it always feels a little iffy to mix styling with logic although I realize that's always particularly challenging with carousels. Ideally, I feel like the picture element should be a "first-class citizen" that's supported by a modern carousel. Again, I understand that that's probably quite challenging in reality.

Of course, this discussion also extends to the general case where the slides change aspect ratio depending on the viewport - especially when the slides contain content other than images. One common example, I'd imagine, is a testimonial slider. While you'd probably want each testimonial/slide to be the same height to avoid a janky/jumpy layout, you might want to change the aspect ratio on smaller screens due to the text being taller.

This is exactly my case, a testimonials with image and text underneath, the text generally has dynamic height.
What do you suggest in this case?

[cyphire]

I have the same problem as above... I removed owl for this and I love it. Unfortunately I have dynamic text below my image and also need to know how to handle it.

@mvasin I understand where you are coming from. We looked at it more from the point of view of people using the component. People generally know or have the size of the images that will be in the carousel.

I disagree with this. This is a powerful carousel. It shouldn't be relegated to someone with fixed images of a specific size.

[cyphire]

I have a pretty nice example for being able to dynamically show different size slides. In my case I am doing blog posts (articles). I have just implemented it with actually a data driven group of carousels. I shortened an example project to just do one carousel for anyone that needs this functionality.

[cyphire]

example

I don't know if you still need this ability but I created a nice site using it.

[tim-steele]

@cyphire we are open to it if you want to do a draft pull request for review?