Proposal: Adding image support

ghost commented 5 years ago

This little change adds optional image support or should this go into the additional props?

If agreed, we have to alter the docs too to reflect the change.

mattstein commented 5 years ago

I appreciate this @mikewink and I'm on the fence about it. You can use additionalProps right now as you suggest, and I'm just wondering if the template parameter should either...

a) be imageUrl to clarify that only a URL (and not an Asset reference, for example) is expected or
b) remain as image and attempt to gracefully handle an Asset reference somehow

Your change is a straightforward way of having the Twig method's interface reference Snipcart's product definition, I'm just not sure if it should be more Asset-sensitive in this case or if I'm overthinking it.

ghost commented 5 years ago

I used 'image' as the key because it's the name used by Snipcart. I don't have any problem with the naming whatsoever. I would not mind being more specific. That's ultimately up to you.

mattstein commented 5 years ago

I'm just overthinking it out loud. I appreciate the suggestion and I'll merge+document for the next release.

mattstein commented 5 years ago

Updated in 1.0.5, noting this addition and clarifying all options in the docs.

verbb / snipcart

Proposal: Adding image support #9