search

Raynos / min-document

A minimal DOM implementation
MIT License
109 stars 27 forks source link

Improve serialization with regards to attributes and style-settings #19

Closed fangel closed 9 years ago

fangel commented 9 years ago

Hi,

I was trying to play around with server-side rending DOMDocument's created using min-document, and I stumbled upon the fact that serializeElement doesn't handle attributes or style-settings very well.

I have attempted to add some functionality to cover this - but it seems like there might be some left-over stuff from a previous version of how attributes where handle, maybe?

There is an issue with running the tests in the browser, as they tend to serialise elements slightly different. E.g. <div style="color:red;"></div> vs <div style="color: red; "></div>. Or how how to handle shorthand style syntax, where browser sometimes expands it.. I'm not sure how thoroughly this implementation should match the browsers?

But anyway, it would be awesome to have serializeElement actually serialise attributes and style-settings as well - and this PR is an effort towards this, but might not be the final solution. So I would love some discussion or input on how this should best be achieved...

Kind Regards Morten

Raynos commented 9 years ago

@fangel

style used to work, see the stylify function.

setAttribute() was recently added and we never added serialization support for it.

Raynos commented 9 years ago

If the tests pass then this lgtm.

Raynos commented 9 years ago

Published as v2.14.0

Raynos commented 9 years ago

Added you as a collaborator.

fangel commented 9 years ago

Yes, I could see that everything was more or less there - I just ensured that a few more things were added to the props-map in properties, and then things worked out well. But I'm not entirely sure that the initial loop over the elements own properties is actually needed - the only one it seems to find and use is the style one - even className is handled separately. And all of the element-attributes needs special cases too.. So I might try and refactor it some further.

My plans would be to 1) Add more tests of the serialisation in test-document.js 2) I might skip the serialisation-tests when run in the browser - because I think it's unrealistic to 100% match the output-format char-for-char across different browsers. 3) Then maybe check and see if the properties-method can be simplified to not rely on the isProperty-method.

I would do this as a new PR against a branch under this repo if you think this is a reasonable strategy...

Raynos commented 9 years ago

@fangel

Consider writing serialization tests that do indexOf() checks rather then equality checks, i.e. search for substrings.

Also free free to make pull requests against master :)

If you do want to work on a feature branch, be my guest.