Skip to content

allow string props for image width/height #8

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

Closed
wants to merge 3 commits into from

Conversation

Roilan
Copy link
Collaborator

@Roilan Roilan commented Apr 28, 2016

Allows Image height / width a string prop because auto is a common use case to maintain aspect ratio.

@chromakode
Copy link
Owner

Thanks @Roilan! I've not heard of width="auto" in an HTML attribute before. Been looking for an example & checking the specs, but haven't been able to find it. Have you seen this in the wild?

@Roilan
Copy link
Collaborator Author

Roilan commented Apr 29, 2016

You have a good point. I don't think I've seen people do it outside of using CSS for width: auto. The alternative would be to not make height / width a requirement and use the headCSS prop.

@chromakode
Copy link
Owner

I think that sounds like the way to go, though there is this caveat to using images without sizes in the markup: https://www.campaignmonitor.com/blog/email-marketing/2007/06/always-include-the-width-and-h/

@Roilan
Copy link
Collaborator Author

Roilan commented Apr 29, 2016

Hmmm. You have a good point. I'm not entirely sure what the right decision is here. While auto as a width or height attribute might not be in spec, it does work and useful in some cases (keeping aspect ratio). Even if we had a default 1px to meet the above, we wouldn't be able to override it with CSS without !important and that's not a good solution either.

@chromakode
Copy link
Owner

It seems like a good idea to include a starting width/height even if the image is going to be scaled, and progressively enhance via CSS if it's supported. Overriding the width/height via CSS !important actually seems like a decent approach for a responsive stylesheet. What do you think?

@Roilan
Copy link
Collaborator Author

Roilan commented Apr 30, 2016

Out of the options we have mentioned, I think that's the best thing to do. We should also add a note in the documentation mentioning this. Wouldn't want someone spending a great amount of time trying to debug this. I'm good to make the adjustments if you're on board.

@chromakode
Copy link
Owner

That sounds great, thank you!

@Roilan
Copy link
Collaborator Author

Roilan commented May 2, 2016

Closing see #9

@Roilan Roilan closed this May 2, 2016
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.

2 participants