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

Optional image / height props and documentation note #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Roilan
Copy link
Collaborator

@Roilan Roilan commented May 2, 2016

@chromakode Updates from our discussion from #8

An image, without any pesky borders, outlines, or underlines by default. Requires a `src` prop, and `width` and `height` to be set. You can override the default styles (such as adding a border) using the `style` prop.
An image, without any pesky borders, outlines, or underlines by default. Requires a `src` prop. Optional `width` and `height` props are available. You can override the default styles (such as adding a border) using the `style` prop.

`height` and `width` props default to `1px`. If you require a response image, for example, having `300px` of height and `auto` width. We recommend adding the number of pixels directly as a prop and having `auto` in your `headCSS`.
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: "response" -> "responsive"

@chromakode
Copy link
Owner

Defaulting to 1px seems unwise to me; it's not an obvious behavior if you haven't used the module before and create an image with no dimensions. I would prefer that we require authors to specify an intentional width/height value (since this is email and sometimes CSS is not supported).

@Roilan, have you encountered a use case where specifying know dimensions is undesirable or not possible? I think it might make sense to be opinionated here: keep the dimensions requirements and document the use case (as you have in this PR).

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