fix: updates <Picture /> to pass HTML attributes down to the <img /> element #5038

emmanuelchucks · 2022-10-10T13:24:46Z

class for direct img styling, width & height to prevent layout shift, etc need to be passed to the underlying img tag to work.

Closes #3909

Changes

Passes attributes on the Picture component to underlying img
For example, styling with class is reflected on image
Setting width and height to prevent layout shift also now works

Testing

This change only corrects a slight oversight that doesn't need much testing.

Docs

/cc @withastro/maintainers-docs for feedback!

This is the expected behavior for most users, just like next/image
https://nextjs.org/docs/api-reference/next/image#other-props

`class` for direct img styling, `width` & `height` to prevent layout shift, etc need to be passed to the underlying img tag to work.

changeset-bot · 2022-10-10T13:24:49Z

🦋 Changeset detected

Latest commit: 01c72fd

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

panwauu · 2022-10-10T14:22:31Z

The width and height attributes were removed in #4797

Related discussion:
#3909 (Mentioned by you)
#4985 (Problem with passing styles to image and picture)

panwauu · 2022-10-10T14:30:06Z

How would you handle changing sizes?

So for example on a smaller screen you load the 500x300 version instead of 1000x600 would you update the properties? I guess with css width: 100%; height: fit-content; you could still work.

I think it could be confusing to pass on properties like class to the img

I would assume that class on Picture applies the class to the pictue tag as usual. Maybe we could have a different asstribute like imgClass to pass a class to the img tag but this would result in a problem with the css scoping.

Overall, I think that a change is necessary to the component, but we need to think about how to do it.

emmanuelchucks · 2022-10-10T17:10:42Z

For the first issue raised:

When {...attrs} is passed to <img /> tag,

<Picture
  src="https://images.unsplash.com/photo-1620189507187-1ecc7e2e9cff?ixlib=rb-1.2.1&dl=austin-wilcox-z_U6bPp_Rjg-unsplash.jpg&q=80&fm=jpg&crop=entropy&cs=tinysrgb"
  alt="a cute little puppy"
  widths={[200, 400, 600, 800, 1000, 1200, 1600]}
  sizes="100vw"
  width={3448}
  height={5168}
  aspectRatio={3448 / 5168}
/>

generates the following html

<picture width="3448" height="5168" class="astro-DDSVHVWK">
  <source
    type="image/avif"
    srcset="
      /_image?f=avif&w=200&h=300&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb    200w,
      /_image?f=avif&w=400&h=600&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb    400w,
      /_image?f=avif&w=600&h=899&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb    600w,
      /_image?f=avif&w=800&h=1199&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb   800w,
      /_image?f=avif&w=1000&h=1499&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb 1000w,
      /_image?f=avif&w=1200&h=1799&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb 1200w,
      /_image?f=avif&w=1600&h=2398&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb 1600w
    "
    class="astro-DDSVHVWK"
    sizes="100vw"
  />
  <source
    type="image/webp"
    srcset="
      /_image?f=webp&w=200&h=300&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb    200w,
      /_image?f=webp&w=400&h=600&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb    400w,
      /_image?f=webp&w=600&h=899&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb    600w,
      /_image?f=webp&w=800&h=1199&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb   800w,
      /_image?f=webp&w=1000&h=1499&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb 1000w,
      /_image?f=webp&w=1200&h=1799&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb 1200w,
      /_image?f=webp&w=1600&h=2398&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb 1600w
    "
    class="astro-DDSVHVWK"
    sizes="100vw"
  />
  <img
    src="/_image?f=webp&w=1600&h=2398&ar=0.6671826625386997&href=https%3A%2F%2Fimages.unsplash.com%2Fphoto-1620189507187-1ecc7e2e9cff%3Fixlib%3Drb-1.2.1%26dl%3Daustin-wilcox-z_U6bPp_Rjg-unsplash.jpg%26q%3D80%26fm%3Djpg%26crop%3Dentropy%26cs%3Dtinysrgb"
    class="astro-DDSVHVWK"
    loading="lazy"
    decoding="async"
    alt="Emmanuel and his wife in native Ghanaian attire"
    width="3448"
    height="5168"
  />
</picture>

It loads the right size at each breakpoint and still prevents layout shift. Is there something I'm missing?

Second issue raised:

I can see how that would be confusing especially to beginners. Maybe we can make it clear in the docs? The picture was never really meant to be styled directly anyway. Considering that alt, loading, decoding, object-fit, object-position, etc only work when passed to <img />. I believe this is the more consistent behavior.

paularmstrong · 2022-10-12T18:29:22Z

I just ran into this and was about to file an issue. I would just like to second @emmanuelchucks responses that this PR impelements what I expect to happen when passing attrs to the Picture component: they should get applied to the <img />, not the <picture> element.

carlcs · 2022-10-13T20:15:10Z

@emmanuelchucks Author of the issue you’re addressing here, so thanks for creating the PR. As explained in the issue I think it’s the right choice to add the attributes to the <img> element.

But why are you still adding them to the <picture> element as well, or was that just an oversight?

- seemed to break some tests - can't deal with fixing that right now, maybe later

emmanuelchucks · 2022-10-15T11:41:52Z

@carlcs It seems to break something in the tests. That was my initial hunch.

tony-sull · 2022-10-18T18:17:02Z

Thanks for getting this PR moving, @emmanuelchucks! I've been busy on other projects the last couple weeks and haven't had much time to work on images, but I know the big discussions right now are related to styling pictures and the width/height attributes

I think the tests are selecting picture elements by ID right now. I'm also +1 on only spreading the attributes onto the img tag, we'd just need to find a different way to select the picture in tests (maybe getById().parentElement?)

tony-sull · 2022-10-18T19:39:20Z

Pushed up a change that only includes attributes on the <img> + test fixes to update how the test elements are selected

I like this change and think it takes care of both current pain points (styling the picture & layout shift), going to leave this open for a bit to get more feedback in case anyone has concerns with attributes no longer going on <picture>

matthewp · 2022-10-18T20:04:35Z

lgtm

paularmstrong · 2022-10-20T19:51:13Z

Thank you all for the work on this!

fix: also pass attrs to underlying img

13e83df

`class` for direct img styling, `width` & `height` to prevent layout shift, etc need to be passed to the underlying img tag to work.

github-actions bot added the pkg: integration Related to any renderer integration (scope) label Oct 10, 2022

emmanuelchucks added 2 commits October 15, 2022 10:29

remove redundant attr from picture tag

e9daca6

revert to previous commit

99f94b6

- seemed to break some tests - can't deal with fixing that right now, maybe later

Tony Sullivan added 4 commits October 18, 2022 13:28

Merge branch 'main' into patch-1

805786c

only passing attributes to the img

76836bc

adding a note to the README

586dcc7

chore: add changeset

f730b48

tony-sull requested a review from a team as a code owner October 18, 2022 19:06

tony-sull changed the title ~~fix: also pass attrs to underlying img~~ fix: updates <Picture /> to pass HTML attributes down to the <img /> element Oct 18, 2022

Merge branch 'main' into patch-1

01c72fd

tony-sull self-assigned this Oct 20, 2022

tony-sull merged commit ed2dfda into withastro:main Oct 20, 2022

astrobot-houston mentioned this pull request Oct 20, 2022

[ci] release #5135

Merged

emmanuelchucks deleted the patch-1 branch October 20, 2022 22:43

bluwy mentioned this pull request Apr 8, 2023

Fix/scoped css does not apply to Picture #6778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: updates <Picture /> to pass HTML attributes down to the <img /> element #5038

fix: updates <Picture /> to pass HTML attributes down to the <img /> element #5038

emmanuelchucks commented Oct 10, 2022

changeset-bot bot commented Oct 10, 2022 •

edited

Loading

panwauu commented Oct 10, 2022

panwauu commented Oct 10, 2022

emmanuelchucks commented Oct 10, 2022 •

edited

Loading

paularmstrong commented Oct 12, 2022

carlcs commented Oct 13, 2022

emmanuelchucks commented Oct 15, 2022

tony-sull commented Oct 18, 2022

tony-sull commented Oct 18, 2022

matthewp commented Oct 18, 2022

paularmstrong commented Oct 20, 2022

fix: updates <Picture /> to pass HTML attributes down to the <img /> element #5038

fix: updates <Picture /> to pass HTML attributes down to the <img /> element #5038

Conversation

emmanuelchucks commented Oct 10, 2022

Changes

Testing

Docs

changeset-bot bot commented Oct 10, 2022 • edited Loading

🦋 Changeset detected

panwauu commented Oct 10, 2022

panwauu commented Oct 10, 2022

emmanuelchucks commented Oct 10, 2022 • edited Loading

For the first issue raised:

Second issue raised:

paularmstrong commented Oct 12, 2022

carlcs commented Oct 13, 2022

emmanuelchucks commented Oct 15, 2022

tony-sull commented Oct 18, 2022

tony-sull commented Oct 18, 2022

matthewp commented Oct 18, 2022

paularmstrong commented Oct 20, 2022

changeset-bot bot commented Oct 10, 2022 •

edited

Loading

emmanuelchucks commented Oct 10, 2022 •

edited

Loading