Shopify / ui-extensions

MIT License
254 stars 35 forks source link

Image component with fit='contain' should constrain the height of the image inside the container #940

Open jduff opened 1 year ago

jduff commented 1 year ago

Please list the package(s) involved in the issue, and include the version you are using

checkout-ui-extensions

Describe the bug

When using an image component with fit='contain' the image should be contained within the container.

Steps to reproduce the behavior:

  1. Set up an Image with fit contain inside an InlineLayout with fixed sizes. ex (using htm templates):
    <InlineLayout spacing='extraTight' maxInlineSize=${20} maxBlockSize=${100} overflow='hidden'>
    <Image fit='contain' source="example.png" />
    </InlineLayout>

Expected behavior

The image should be resized to the height of the container instead of overflowing out of it.

Screenshots

Screenshot 2023-05-16 at 3 49 53 PM

Additional context

Constraining the width works fine, I think this is because max-width: 100% is added to the image. Adding max-height: 100% when using fit='contain' should fix this issue.

I'm open to other solutions here, but no combination of InlineLayout / BlockLayout / View with various maxInlineSize or maxBlockSize have worked for me to constrain the height of the image. Another solution would be adding maxWidth and maxHeight properties to the Image component as well.

ryan-ludwig commented 1 year ago

Thanks for the report, @jduff . We're adding it to our backlog for prioritization.

edhgoose commented 1 month ago

Hi @ryan-ludwig - do you have any update on this issue?

It appears that I have the same experience - maxInlineSize seems to work exactly as I'd expect, but like @jduff I find that no combination of maxBlockSize applies any change to the imagery.

When I apply maxInlineSize, I get CSS applied to the component which adjusts the behaviour as I'd expect. But, if I use maxBlockSize I can't see any CSS even applied that would influence the size.

ryan-ludwig commented 1 month ago

Hey @edhgoose, thanks for the info. We've got it triaged but haven't started work on it yet.