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 srcset option to image_tag helper #29349

Merged
merged 1 commit into from Jul 1, 2017

Conversation

@robertomiranda
Copy link
Contributor

@robertomiranda robertomiranda commented Jun 3, 2017

Summary

Adding srcset option to image_tag in order to add Responsive Images.

Usage:

image_tag("icon.png", srcset: { 'pic_640.jpg' => '640w', 'pic_1024.jpg' => '1024w', 'pic_1980.jpg' => '1980w' }, sizes: '100vw', class: 'my-image')
#=> <img src="/assets/ants_1980.jpg" srcset="/assets/pic_640.jpg 640w, /assets/pic_1024.jpg 1024w, /assets/pic_1980.jpg 1980w" sizes="100vw" class="my-image">
@rails-bot
Copy link

@rails-bot rails-bot commented Jun 3, 2017

r? @matthewd

(@rails-bot has picked a reviewer for you, use r? to override)

@matthewd
Copy link
Member

@matthewd matthewd commented Jun 4, 2017

I think we should respect :skip_pipeline for these images. No need to worry about controlling it separately for each -- just have them all use the value from the main options.

I like that this works with both a hash or an array-of-arrays; let's cover both of those in the tests.

We definitely need to allow a value that's already a string to pass through safely, too, though -- people will already be using it in that way, and we don't want to break them.

@robertomiranda
Copy link
Contributor Author

@robertomiranda robertomiranda commented Jun 4, 2017

@matthewd sure, I'll be working on that, thanks for the feedback 😁

@robertomiranda robertomiranda force-pushed the robertomiranda:responsive-images branch 7 times, most recently Jun 10, 2017
@robertomiranda
Copy link
Contributor Author

@robertomiranda robertomiranda commented Jun 11, 2017

@matthewd I think is ready again for review, could you take a look again??

actionview/lib/action_view/helpers/asset_tag_helper.rb Outdated
@@ -237,6 +243,13 @@ def image_tag(source, options = {})
options[:alt] = options.fetch(:alt) { image_alt(src) }
end

if options[:srcset] && !options[:srcset].is_a?(String)
options[:srcset] = options[:srcset].map do |src, size|
src_path = path_to_image(src, skip_pipeline: options.delete(:skip_pipeline))

This comment has been minimized.

@robotdana

robotdana Jun 12, 2017
Contributor

deleting the option inside the loop here seems like it won't do what you want

This comment has been minimized.

@robertomiranda

robertomiranda Jun 13, 2017
Author Contributor

yeah you are right

@robertomiranda robertomiranda force-pushed the robertomiranda:responsive-images branch Jun 13, 2017
@robertomiranda robertomiranda force-pushed the robertomiranda:responsive-images branch to 43efae2 Jun 29, 2017
@robertomiranda
Copy link
Contributor Author

@robertomiranda robertomiranda commented Jun 29, 2017

@matthewd could you take a look again over this PR??

@matthewd matthewd merged commit 43efae2 into rails:master Jul 1, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
matthewd added a commit that referenced this pull request Jul 1, 2017
Add srcset option to image_tag helper
@robertomiranda robertomiranda deleted the robertomiranda:responsive-images branch Jul 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants