search

evilstreak / markdown-js

A Markdown parser for javascript
7.7k stars 863 forks source link

In image tags, put the `src` attribute first. #126

Closed eviltrout closed 11 years ago

eviltrout commented 11 years ago

This one might be more controversial, so I'm happy to discuss it.

MDTest1.1 wants all src attributes to be first. This is obviously not possible in the JsonML representation because Javascript objects have no order to their key/values. However, we can easily check if a src attribute is present and output it first in the rendering phase.

At first I thought this was silly and not worth doing, but after speaking to my team I realized that if we want HTML that reads nicely, the src attribute of an image should be first. You can debate secondary attributes, but for good looking HTML, the src is the most important and should be read first.

evilstreak commented 11 years ago

I see no problem with this approach.

XhmikosR commented 11 years ago

I personally always put src first and the rest attributes after that, so I think it makes sense.

ashb commented 11 years ago

One comment before I get onto the changeset: features.t.js is designed to just be a runner for the files under tests/features/* so this test should go in another file - either regressions.t or create a new file of your own naming

One thing that the current test suite does get wrong is that the features.t.js test goes from MD text to HTML json-ml - so features should check to Markdown JsonML and then separate tests should cover the JsonML->JsonML conversion and yet more covering the JsonML->HTML test - maybe a new tests/html_renderer.t.js or similar would be the right place for this sort of test.

Now onto the actual change itself. So there's to things:

MDTest1.1 wants all src attributes to be first

So one of the things we wanted was our test suite not to fail just because someone introduced a white space or an order change - this is why the feature test checks the HTML JsonML not the HTML text. So if this is the only reason you want this change then: no, and if we want to check against mdtest1.1 then we should convert it's html to JsonML - after all the order or whitespacing in HTML doesn't matter

If you want it cos you like to view the source and have it look pretty I'm actually more on-board with. Though fairly soon after we'll want href first, then height -> width ;)

eviltrout commented 11 years ago

@ashb I have moved the test to be in html_renderer.t.js as suggested.

MDTest is not the only reason I want this. Actually I argued internally on our team that the position of src should not designate a failing MDTest. Then I was convinced that for aesthetic reasons, viewing source should show the src attribute first and I came around.

Personally I wouldn't be against href first, but height -> width seems crazy to me :P

evilstreak commented 11 years ago

Merged in 4bc5058dcb40771b570262d9eeb8d296fb991b58