Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DOM-compatible method insert_before #664

Closed
Phrogz opened this issue Apr 24, 2012 · 3 comments
Closed

Add DOM-compatible method insert_before #664

Phrogz opened this issue Apr 24, 2012 · 3 comments

Comments

@Phrogz
Copy link

Phrogz commented Apr 24, 2012

If you want to insert a node as the first child of another you currently have to write code like:

dad.children.empty? ? dad << contents : dad.children.first.before(contents)

If Nokogiri added a Nokogiri::XML::Node#insert_before method like the DOM 2 Core method Node.prototype.insertBefore() then the above code would be far simpler and DRY:

dad.insert_before( contents, dad.children.first )
@Phrogz
Copy link
Author

Phrogz commented Apr 24, 2012

FWIW, I've repeatedly been surprised that Nokogiri's method names do not match (or alias) the DOM methods. I totally appreciate the convenience of things like a.add_next_sibling(b) over a.parentNode.insertBefore(b,a.nextSibling), but for those who live in the worlds of both HTML DOM and Nokogiri it would be nice to ensure that all (or at least common) functionality and names from DOM are available almost completely verbatim in Nokogiri.

@flavorjones
Copy link
Member

@Phrogz -

Thanks for suggesting this! It's clearly a hole in Nokogiri's API that we'll fill.

Nokogiri was never intended to be a 1-to-1 DOM mapping. There's a reason why we ripped off hpricot's API and not the W3C committee's API. And it's the same reason that jQuery exists: http://en.wikipedia.org/wiki/Design_by_committee

I mean, insertBefore does exactly what it says ... as long as you mean `insert a child node before this other child node" ... unless you don't have another child, in which case it's not inserting before anything.

How about this method instead:

dad.prepend_child contents

or maybe an option on the existing method like:

dad.add_child contents, :prepend => true

@Phrogz
Copy link
Author

Phrogz commented Apr 27, 2012

Both of your suggestions look good. If I had to pick one over the other I'd personally lean towards the first one, though both would be nice. Even nicer, though, would be to modify add_child fully to support the a positional anchor which may be nil:

dad.add_child contents, prepend:true  # prepend
dad.add_child contents, prepend:false # append
dad.add_child contents, append:true   # append
dad.add_child contents, append:false  # prepend
dad.add_child contents, before:foo    # before foo
dad.add_child contents, before:nil    # append
dad.add_child contents, after:foo     # after foo
dad.add_child contents, after:nil     # prepend

While the DOM bindings are admittedly weird and often inconvenient, I find that having the ability to specify an anchor element or null often DRYs up code, handing the edge case with the exact same code as the base case. An explicit append_child and prepend_child would cover some of the edge cases nicely, but I'm not convinced that they handle all situations elegantly.

deleted a use case that did not, in fact, make sense

knu added a commit that referenced this issue Oct 30, 2013
@knu knu closed this as completed Oct 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants