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

Our-img height #65

Open
kahlan88 opened this issue Jun 7, 2023 · 1 comment
Open

Our-img height #65

kahlan88 opened this issue Jun 7, 2023 · 1 comment

Comments

@kahlan88
Copy link

kahlan88 commented Jun 7, 2023

We're using <our-img> with media-item and responsive widths and heights.

According to example 4 in the readme - the helper should output height attribute and in the srcset on <source> tags.

<our-img media-item="Model.Image" width="200" width--s="400" width--m="600" cropalias="mobile" cropalias--m="desktop" />
<picture>
    <source srcset="/media/path/image.jpg?width=600&amp;height=300&amp;rnd=133087885756657361" media="(min-width: 768px)" width="600" height="300">
    <source srcset="/media/path/image.jpg?width=400&amp;height=400&amp;rnd=133087885756657361" media="(min-width: 576px)" width="400" height="400">
    <img alt="Some alt text" width="200" height="200" src="/media/path/image.jpg?width=200&amp;height=200&amp;rnd=133087885756657361" loading="lazy" decoding="async" fetchpriority="low">
</picture>

Our example code:

<our-img
        media-item="@image"
        width="540"
        height="480"
        width--s="590"
        height--s="660"
    />

Output:

<picture>
    <source srcset="/media/path/image.jpg?width=590&amp;rnd=133282895487345646" media="(min-width: 768px)" width="590">
    <img alt="Some alt text" width="540" height="360.02313624678663" src="/media/path/image.jpg?width=540&amp;rnd=133282895487345646" loading="lazy" decoding="async" fetchpriority="low">
</picture>

Expected:

<picture>
    <source srcset="/media/path/image.jpg?width=590&height=480&amp;rnd=133282895487345646" media="(min-width: 768px)" width="590" height="480">
    <img alt="Some alt text" width="540" height="360.02313624678663" src="/media/path/image.jpg?width=540&amp;rnd=133282895487345646" loading="lazy" decoding="async" fetchpriority="low">
</picture>

We need slightly different aspect ratios based on screen size. Is it possible?

I think that height is missing from the output. Am I missing something or is this a defect?

appsettings.json:

"Our.Umbraco.TagHelpers": {
    "OurIMG": {
      "MobileFirst": true,
      "MediaQueries": {
        "Small": 768,
        "Medium": 1024,
        "Large": 1280,
        "ExtraLarge": 1440,
        "ExtraExtraLarge": 1680
      },
      "UseNativeLazyLoading": true,
      "ApplyAspectRatio": false,
      "AlternativeTextMediaTypePropertyAlias": "altText"
    }
  }
@kahlan88
Copy link
Author

kahlan88 commented Jun 8, 2023

Having looked into the code, I can see that when MediaItem is provided, the height gets omitted and the code takes only the crops into account.

It applies height when providing src instead.

I think it would be great if height was used if provided. There could be a flag to say "ignore crops", but I think just the height being present could be used to tell the tag helper to respect it and ignore crops?

Note: not all sites even use crops - for example none of the sites I generally work on utilises this much, and we'd almost always want to use height to override crops when providing media-item, so it would be really useful to have the option. I will see if I can PR this change.

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

No branches or pull requests

1 participant