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

To move the element with after (before) method. #166

Closed
ritalin opened this issue Feb 26, 2013 · 11 comments
Closed

To move the element with after (before) method. #166

ritalin opened this issue Feb 26, 2013 · 11 comments

Comments

@ritalin
Copy link

ritalin commented Feb 26, 2013

In jQuery, a specified element is not copied but moved with after (before) method whenever the element has been already exsited in .

In Cheerio, however a specified element has been always copied.

Is this behaviour bug, or specification?

Best, regards.

@ritalin
Copy link
Author

ritalin commented Feb 26, 2013

If necessary, a pull request would be sent latter (sorry fixed code has not been yet written...).

@fb55
Copy link
Member

fb55 commented Feb 26, 2013

DOM elements can only have a single parent element, so jQuery gets the moving for free. Fixing this bug should be fairly easy, check for the parent reference, if it exist, remove the element from it's parent, add it to the new parent, update the reference.

@matthewmueller
Copy link
Member

@ritalin do you mind explaining the issue your having with copying? That way we can make sure to address the problem.

@fb55
Copy link
Member

fb55 commented Feb 26, 2013

As far as I understand it, the issue isn't copying, but maintaining two references; the element isn't removed from it's old parent.

@ritalin
Copy link
Author

ritalin commented Feb 27, 2013

@matthewmueller To move an element in children, now the before (after) method has used. Therefore an element in children was increased.
In jQuery, (after) method has been implemented in using insertBefore method internally. So the element is meved automatically.
Currentlly, I'v been using Cheerio monkey-patching it.

@ritalin
Copy link
Author

ritalin commented Feb 27, 2013

@fb55 Sure, An element is not copied but had two reference.

@matthewmueller
Copy link
Member

Hmm.. can you post some code? Including your monkey-patch that solves the issue. I feel like this would affect a bunch of other people if this was passing references.

@ritalin
Copy link
Author

ritalin commented Mar 6, 2013

Avoiding to modify the codebase of cheerio, migration code is prepare for monkey-patch

Following is a part of the migration code

var cheerio = require('cheerio');

var _after = cheerio.prototype.after;
cheerio.prototype.after = function() {
  var elems = Array.prototype.slice.call(arguments);

  return _after.apply(
    removeTargetElements(this, elems), arguments
  );
}

var _before = cheerio.prototype.before;
cheerio.prototype.before = function(elements) {
  var elems = Array.prototype.slice.call(arguments);

  return _before.apply(
    removeTargetElements(this, elems), arguments
  );
}

var removeTargetElements = function(self, elems) {
  self.each(function(i, el) {
    var siblings = el.parent.children;

    var len = elems.length;
    for (var k = 0; k < len; ++k) {
      var index = siblings.indexOf(elems[k][0]);

      if (index >= 0) {
        siblings.splice.apply(siblings, [index, 1]);
      }
    }
  });

  return self;
}

module.exports = cheerio;

I think that can fix more smartly...

@tjconcept
Copy link

If I do .append(domElem) and later domElem.remove() i would expect the element to be removed, but .append seems to be copying instead of moving the reference - is this a related bug/feature?

It makes it very difficult to toggle an element.

@tjconcept
Copy link

I'm wrong. The problem was that I was doing .append multiple times on the same element, which makes a copies. This is however still a difference from jQuery.

@fb55
Copy link
Member

fb55 commented Apr 8, 2014

This was recently fixed.

@fb55 fb55 closed this as completed Apr 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants