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

Add support for additional image attributes #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

notnek
Copy link

@notnek notnek commented Aug 2, 2024

Description

Adds support for setting additional attributes beyond src on the image node HTML output.

Reason/Reference

Currently the implementation doesn't allow for alt text (or any other attribute) to be set.

Thanks for the excellent work on this gem!

@notnek
Copy link
Author

notnek commented Dec 9, 2024

@jeffmcfadden @chadwilken hey y'all, I wanted to ping to see what you thought about this PR? It doesn't look like I can select any one for a review. Thanks!

@chadwilken chadwilken requested a review from a team December 9, 2024 21:29
@chadwilken
Copy link
Collaborator

@jeffmcfadden @chadwilken hey y'all, I wanted to ping to see what you thought about this PR? It doesn't look like I can select any one for a review. Thanks!

@notnek thanks for opening a PR. I think this looks good. I'm trying to think if there would be any security vulnerabilities possible by allowing all attributes to be passed to image_tag. @jeffmcfadden or @DustinFisher any thoughts on that?

@EdwardBarton EdwardBarton requested a review from a team December 13, 2024 20:15
@@ -12,7 +12,7 @@ def include_empty_content_in_json?
end

def to_html
image_tag(src)
image_tag(src, attrs.except("src"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if the goal is setting alt that we just allow alt or at minimum the allow options that can be passed to the image_tag helper. https://api.rubyonrails.org/v7.2.2/classes/ActionView/Helpers/AssetTagHelper.html#method-i-image_tag

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to change it to only set alt for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be great, thanks!

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

Successfully merging this pull request may close these issues.

3 participants