Allow width, height, and valign attributes on img tags

[rreusser]

I filed an issue (in hindsight, for the wrong repo) at npm/newww#1107. In short width, height, and valign attributes are allowed on github and disallowed on npmjs.com which makes it rather difficult to achieve visual consistency between the two.

Some examples:

• https://www.npmjs.com/package/gulp-markdown-equations vs. https://github.com/rreusser/gulp-markdown-equations (update: tried SVG on this one, proxying it through rawgit.com to get around strange and seemingly unnecessary github security restrictions)
• https://www.npmjs.com/package/fly vs. https://github.com/flyjs/fly
• https://www.npmjs.com/package/ndarray-blas-level1 vs. https://github.com/scijs/ndarray-blas-level1

Basically any image with a fixed width and height loses it when published to npm (i.e. the only non-css way to use retina images—and I'd use SVG except SVG has its own issues on github). I know this will affect the look of quite a few READMEs on npm so I don't take this lightly, but I think there's a pretty good argument to be made that will only fix visual inconsistencies rather than creating them since it's not clear why you'd set these attributes and prefer that they not be obeyed.

Tests are added. I can't think of any other issues like security that this would interfere with.

Thanks for your consideration!

[rreusser]

Note: I added and then removed <p> tags + the align attribute since it's a common github-friendly way to center-align things, but I'll take this one step at a time since that may cause more sanitization issues (although I suspect it's probably fine and bad <p> tags get sanitized anyway).

[zeke]

Looks good to me. I don't have commit bit on this repo any more, though.

[rreusser]

Thanks, @zeke! We'll see! 👍

[sindresorhus]

@othiym23 Can someone merge this? It's annoying that logos and stuff goes fullwidth.

Examples:

• https://www.npmjs.com/package/alex
• https://www.npmjs.com/package/hasha

[othiym23]

This isn't a code base I have a lot of contact with, so it would be better for @rockbot, @jefflembeck, or @aredridel to take a look at landing this one. It may be a bit, too, as they're heads down right now.

[bcoe]

@soldair would be a good person to reach out to as well, he's working on patching some dependencies of the library right now.

[sindresorhus]

An additional note, GitHub applies a max-width:100%; to images to make sure the user can't set a width wider than the page. You should probably do the same on npmjs.com before deploying this.

[rreusser]

@sindresorhus perhaps already in place? See: https://github.com/npm/newww/blob/master/assets/styles/markdown.styl#L63

[soldair]

lgtm - ill check this out after i resolve another issue

[wooorm]

👍

[mattdesl]

Any progress on this @soldair?

Particularly bad case below.

npm:

github:

[aredridel]

LGTM too. Our max-width is already 100% so good to go.

[mattdesl]

🎉 thanks!

[rreusser]

The inspector and a search (link above) suggested that markdown.styl in npm/newww contains the max-width property in question. I'd do a bit more sleuthing and track down the fix, but that's my best guess for what we're talking about. Am I misunderstanding the level on which this needs to happen?

[rreusser]

Oops. Slow on the comment. 😄

[rreusser]

Thanks for the merge! 👻

[aredridel]

You're welcome! We'll roll this out with the next updates, probably!