Generate text node if not given an html tag. #9

Merged
merged 2 commits into from Nov 5, 2013

4 participants

@timoxley
Components member

domify('some text') currently throws " No elements were generated."

If it is sensible for domify to generate a text node in this case, here is some code that does it.

@timoxley timoxley referenced this pull request Oct 13, 2012
Closed

generating text nodes #8

@tj
Components member
tj commented Oct 25, 2012

hmm tough call I think this should be opt-in as well, maybe what we should do is domify(str, { text: true, list: true }) etc to opt-in to node lists and/or text

@timoxley
Components member

Sounds good, though perhaps options could be the first param so you can do something like domify = domify.bind(domify, options), (there's no way to skip a param in a bind is there?) or some other way of configuring these as defaults e.g. via a constructor.

@tj
Components member
tj commented Oct 26, 2012

i'd leave it as an object in this case i think, localizing the options when domify() is called would improve code readability anyway

@jkroso

I think this should be the default. It seems reasonable to convert text to text nodes. People can wrap this module if they want to consider it an error and some people will be wrapping it anyway for the list vs single node thing that was just changed

@jonathanong

@timoxley still want this? i'd prefer for it to be default as well, but i guess TJ wants it as an option. either way, needs a rebase.

@timoxley
Components member

@jonathanong rebased. I can't see how it hurts as a default TBH

@jonathanong

yeah maybe we should just make it default. TJ isn't watching this repo anymore so i don't think he cares :P

also, still can't merge.

@timoxley
Components member

@jonathanong ok try again

@jonathanong jonathanong merged commit 141544e into component:master Nov 5, 2013
@timoxley
Components member

TJ isn't watching this repo anymore so i don't think he cares

:(

@jonathanong

@timoxley which also means @visionmedia doesn't know to publish 1.1.0 to npm for you :(

@jkroso

will document.createTextNode(html) not work cross platform?

http://quirksmode.org/dom/core/ seems fine. want to change it?

sure I'll send a PR

Components member

Yep, good call. Just thinking this is probably more secure too, using .innerHTML you could theoretically inject elements where the user was expecting plain text though tricking that initial regex

Components member

👍 peer review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment