takafumir / rails_amp

RailsAmp is a Ruby on Rails plugin for AMP(Accelerated Mobile Pages).
MIT License
65 stars 26 forks source link

Add allowed class attribute to image tag #6

Closed federicomoretti closed 7 years ago

federicomoretti commented 7 years ago

This adds the class attribute to the image tag helper.

takafumir commented 7 years ago

Hi, thank you for your contribution. I'm so sorry, but I can not merge your pull request. The reason is that AMP does not support some kinds and names of css styles. https://www.ampproject.org/docs/guides/responsive/style_pages

RailsAmp's image_tag helper overrides the Rails built-in image_tag helper, and it doesn't know what css styles are used in image_tag 'class' attributes in user's existing normal html views in advance. If you need to set a 'class' attribute in an amp-img tag, please create your own amp template and use the amp-img tag with a 'class' attribute manually.

jgillman commented 7 years ago

This could just be a fundamental difference of view, but the way I see it since the class attribute itself is allowed it should be allowed by the gem. If the user is writing invalid styles that's something that's outside the scope of the helper, in my opinion.

If the objective of the gem is to prevent the user from writing invalid AMP pages then, yes, class shouldn't be allowed. That said, built in Rails helpers are there to assist writing code, not to prevent the user from potentially writing invalid code.


As an aside, AMP examples often include the class attribute in amg-img tags:

takafumir commented 7 years ago

Hi jgillman. Thank you for the comment. I reconsidered and now I think you and federicomoretti are right. I'm gonna reopen this pull request and merge it. Thanks you two. 🙇

jgillman commented 7 years ago

Thanks @takafumir!!