0.4 pre #4

Closed
wants to merge 10 commits into
from

Projects

None yet

2 participants

@lauriro
lauriro commented Aug 14, 2013

rebased ontop of master with revert of Raynos changes

I continued with single file in lauri branch and have implemented all essential features.

The maximum numbers of lines in one file is arguable but for me 300 lines seems acceptable.

What do you think - is it light weight enought and fits into scope?

I did not understand your documentation file docs.mli so I didn't touch master branch.

I also removed newlines from toString - there are no newlines and spaces in browser document methods.

@Raynos
Collaborator
Raynos commented Aug 14, 2013

@lauriro since innerHTML does not put whitespace or new lines in things when you create using dom manipulation the whitespace removal is good.

The docs.mli file is based on jsig with minor syntax deviations.

I think your code fits within scope.

The options are either to take your PR as is or do style normalization + factor it across one file per interface (Node, Element, innerHTML algorithm) whilst adding more tests.

@Raynos
Collaborator
Raynos commented Aug 14, 2013

Also apologies for splitting it up into multiple files whilst you had a branch, i only realized how annoying that is after I did it.

@lauriro
lauriro commented Aug 14, 2013

In my opinion there are no reasons for one file per interface (files with 7 line for Text and DocumentFragment).

About style normalization - it would be good to set up some style guidelines, so you don't have to fix my code.
As you can guess, for me my style seems quite acceptable :)

At the moment I don't see any missing feature and the current state is sufficient.
Minor optimizations would be mostly style changes and matter of taste - no real reason for performance optimization.

More tests and documentation would be useful for others

@Raynos
Collaborator
Raynos commented Aug 14, 2013

One of the reasons for file seperation is the ability to load a specific interface directly

var document = require("min-document")
var Document = require("min-document/document")
var Element = require("min-document/element")

etc.

@lauriro
lauriro commented Aug 14, 2013

I don't see any real use case for this ability, it is difficult to use just part of document.

@Raynos
Collaborator
Raynos commented Aug 14, 2013

I'll try this branch on projects I have that use min-document for rough integration testing and I'll try to make a list of back compat breaking changes.

Then assuming no bugs I'll merge this in (without refactoring) and publish as 0.4 or 1.0 depending on back compat changes.

@lauriro lauriro closed this Mar 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment