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

[object.crop] Allow images to fill container #278

Closed
dnwhte opened this issue Mar 6, 2017 · 14 comments
Closed

[object.crop] Allow images to fill container #278

dnwhte opened this issue Mar 6, 2017 · 14 comments
Assignees

Comments

@dnwhte
Copy link

dnwhte commented Mar 6, 2017

Setting width & height to 100% on .o-ratio__content causes images to scale without maintaining their aspect ratio. Recommend setting min-width and min-height to 100% instead.

@dnwhte dnwhte changed the title Ratio object stretches images Ratio object doesn't maintain image aspect ratio Mar 6, 2017
@dnwhte dnwhte changed the title Ratio object doesn't maintain image aspect ratio Ratio object doesn't scale images proportionally Mar 6, 2017
@florianbouvot
Copy link
Member

@danwhite85 thank you for your feedback.

Setting width & height to 100% on .o-ratio__content causes images to scale without maintaining their aspect ratio.

True, that's what I initially want 😋

Recommend setting min-width and min-height to 100% instead.

I will make some test and keep you informed.

@florianbouvot florianbouvot self-assigned this Mar 7, 2017
@aaronstezycki
Copy link

FYI ... putting object-fit: cover on the child image fixes this problem... but its a property still in development at minute, it's a no show for IE/Edge but ... there are some fixes and polyfills. :(
http://caniuse.com/#search=object-fit, I would def not use this from within Inuit until it decides to drop support for legacy browsers and Edge has an implemented it.

@florianbouvot
Copy link
Member

florianbouvot commented Mar 21, 2017

@aaronstezycki right but like you say we can really use it now without polyfills.

The purpose of the ratio object is to create a flexible aspect ratio.
I find it interesting in many use case to prevent reflow, lazy load, ...

@florianbouvot
Copy link
Member

@danwhite85 After reflection this isn't really a bug, so I think we can close this issue.

For dynamic content (defined by user...) I advice to combine this object with a crop/resize tools for images.

@dnwhte
Copy link
Author

dnwhte commented Mar 21, 2017

Were issues discovered with using min values? If not, I'll probably still make the change in my personal projects. I'd rather not risk having images stretched.

@florianbouvot florianbouvot removed the bug label Mar 21, 2017
@florianbouvot
Copy link
Member

florianbouvot commented Mar 21, 2017

@danwhite85 Using min-width, min-height doesn't seem to work: http://codepen.io/florianbouvot/pen/xqWdZO

As you can see aspect ratio aren't maintain : image 2 (4:3) fill container because of max-width: 100% (defined in elements.images)

So I add a third line that remove max-width but in this case it look like crop object!?!

@anselmh
Copy link
Member

anselmh commented Mar 22, 2017

In fact, I think we shouldn’t specify min-width or max-width here, since this object is specifically thought for fixed ratio content, and therefore should by definition stretch content to the specified ratio if it is not sized like that.

@csshugs csshugs added the bug label Mar 22, 2017
@florianbouvot
Copy link
Member

@anselmh thanks, your explanation is clearer 😋

@danwhite85 can we close this issue ?

@herzinger
Copy link
Contributor

@danwhite85 why would you use a ratio object and be concerned by stretching? If you force an aspect ratio on something that has another, it's supposed to stretch.

@dnwhte
Copy link
Author

dnwhte commented Mar 22, 2017

My concern is really only with images. I like the current functionality of ratio with iframes, video, etc. My use case is a blend between crop and ratio. I want images to fill the space of their parent (without being stretched) and the remaining is cropped out. Crop only works this way if the image is larger than its container (which I can't always verify).

Maybe a different solution would be a modifier on the .o-crop class e.g:

.o-crop__content--fill{
    min-width: 100%;
    min-height: 100%;
}

@anselmh
Copy link
Member

anselmh commented Mar 23, 2017

@danwhite85 This is only reliably achievable with object-fit: cover. Inuitcss’ crop module is thought to keep an existing crop ratio.

We could indeed add such object-fit classes in an inuitcss plugin—it wouldn’t go into core since Microsoft Edge has still no support for the property yet (it’s in development there though).

@dnwhte
Copy link
Author

dnwhte commented Mar 23, 2017

@anselmh the --fill modifier appears to work as expected in my tests. http://codepen.io/anon/pen/YZLeyN. Not sure what I'm missing.

@nenadjelovac
Copy link
Member

@anselmh I agree with @danwhite85 that it works as expected in the test.

How I see it, o-crop does only one thing: cropping the overflown area, but maintaining the actual ratio of the image.

So having said that, the thing to figure out is if we want the crop object to not mess with the image dimensions. If so, there is no room for o-crop__content—fill. But, I think that modifier is helpful for images smaller than the crop container, so I’d vote to see the proposed modifier added.

@inuitcss/core thoughts?

@florianbouvot
Copy link
Member

@nenadjelovac agree. I never use this object in production because of this kind of issue with image smaller than container...

@florianbouvot florianbouvot removed the bug label May 4, 2017
florianbouvot added a commit that referenced this issue May 4, 2017
@florianbouvot florianbouvot changed the title Ratio object doesn't scale images proportionally [object.crop] Small images doesn't fill container May 4, 2017
@florianbouvot florianbouvot changed the title [object.crop] Small images doesn't fill container [object.crop] Allow images to fill container May 4, 2017
florianbouvot added a commit that referenced this issue Jun 27, 2017
GarethOates pushed a commit to GarethOates/inuitcss that referenced this issue Dec 19, 2017
GarethOates pushed a commit to GarethOates/inuitcss that referenced this issue Dec 19, 2017
GarethOates pushed a commit to GarethOates/inuitcss that referenced this issue Dec 19, 2017
GarethOates pushed a commit to GarethOates/inuitcss that referenced this issue Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants