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

<Picture> component should pass all unknown attributes to the <img> element #3961

Merged
merged 7 commits into from
Jul 19, 2022

Conversation

tony-sull
Copy link
Contributor

Closes #3909

Changes

This updates the <Picture /> component to pass any unused attributes directly to the <img /> element

Current behavior is that the attributes are added to the <picture> but this is a really uncommon use case and breaks accessibility since there is no way currently to add alt to the picture's img

Testing

Tests updated to verify custom <img /> attributes are included

Docs

Integration's README updated with a note on how to add attributes to the picture's <img> element

@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2022

🦋 Changeset detected

Latest commit: ab6a569

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

This PR includes changesets to release 1 package
Name Type
@astrojs/image Patch

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

@tony-sull tony-sull self-assigned this Jul 18, 2022
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jul 18, 2022
Comment on lines 29 to 33
<picture>
{sources.map(attrs => (
<source {...attrs} {sizes}>))}
<img {...image} {loading} {decoding} />
<img {...image} {...attrs} {loading} {decoding} />
</picture>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this play out with Tailwind, which heavily uses the class attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class should be passed down to the img element here, so <Picture class="rounded-sm" /> would result in <picture><img class="rounded-sm" /></picture>

I'd expect styling the img is the common case rather than styling the picture itself, but do you have a use case in mind? Happy to reconsider exposing a way to add props to the picture and img separately if that's useful!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this feels tough to understand for a user. If I add attribute X, how do I know which element that's being applied to? Could you apply it to both?

fwiw I know we discussed the other week that the <Picture> isn't as high of a priority as the other things on our plate for v1.0. I'd love to deprioritize anything <Picture> related and revisit once we're either closer to v1.0 or even post v1.0.

Alternative, I could see a smaller quick fix that just moves the alt attribute to img, but leaves all remaining attributes as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough! Rolled this back to only pass down the alt text to fix accessibility concerns, we can revisit the expected behavior for other attributes later

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JuanM04 the current behaviour makes it impossible apply classes to the child img element, which was the main reason why I opened #3909. The linked reproduction used Tailwind if you want to take a look.

@tony-sull tony-sull merged commit d73c04a into main Jul 19, 2022
@tony-sull tony-sull deleted the feat/picture-attributes branch July 19, 2022 19:21
@astrobot-houston astrobot-houston mentioned this pull request Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: <Picture /> component passes props as attributes to the underlying picture element
5 participants