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

Adding tests for $.clone() #70

Merged
merged 4 commits into from Dec 10, 2015
Merged

Adding tests for $.clone() #70

merged 4 commits into from Dec 10, 2015

Conversation

AVGP
Copy link

@AVGP AVGP commented Dec 9, 2015

Taking care of #67

@LeaVerou
Copy link
Owner

LeaVerou commented Dec 9, 2015

Hey there, thanks for contributing!
However, the main thing that $.clone() does is copy events. Everything else is already handled just fine by the native element.cloneNode(true). So, this is what we need to test for, otherwise you're just testing the browser's native implementation of cloneNode :P

@AVGP
Copy link
Author

AVGP commented Dec 9, 2015

Right, shouldn't code when tired. Let me fix the test :)

@AVGP
Copy link
Author

AVGP commented Dec 9, 2015

...actually... shall I leave this here or remove it more or less entirely and just test events then?

@LeaVerou
Copy link
Owner

LeaVerou commented Dec 9, 2015

I think it doesn't hurt to test other things as well, but the main things to test here are events, on both the cloned element and the descendants.

@LeaVerou
Copy link
Owner

LeaVerou commented Dec 9, 2015

@zdfs I thought we were trying to avoid using other Bliss methods (besides the one being tested) in tests, which I thought was a good idea. Should we document that somewhere, if it's actually a rule?

@AVGP
Copy link
Author

AVGP commented Dec 9, 2015

@LeaVerou Quick question, this test should pass, no?

    var element = $.create("div", { class: "parent",  contents: "This is the parent" });
    element._.delegate("click", "p", function() {
      done();
    });

    var child = $.create("p");
    element.appendChild(child);

    var clone = $.clone(element);

    var ev = document.createEvent("MouseEvent");
        ev.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null);
    clone.childNodes[1].dispatchEvent(ev);

@zdfs
Copy link
Collaborator

zdfs commented Dec 9, 2015

@LeaVerou - Shit. I wasn't aware of that. I've actually been doing the opposite. If that's something you like, we should document it.

@LeaVerou
Copy link
Owner

LeaVerou commented Dec 9, 2015

@zdfs I have no strong opinion, it's just something I noticed as a pattern in other tests. I thought it made sense, since if it doesn't pass, we know for a fact which part of Bliss is problematic. Don't go back changing all your tests though! Let's just document it and try to follow it from now on :)

@AVGP Oh, nice one! It should pass, yes! Btw, please try to avoid using Bliss functions other than the one being tested (see the side discussion with @zdfs)

@zdfs
Copy link
Collaborator

zdfs commented Dec 10, 2015

@LeaVerou - It shouldn't be difficult for me to clean my tests up.

Do we need to do anything else here? Should we wait for the extraneous Bliss functions to come out?

@AVGP
Copy link
Author

AVGP commented Dec 10, 2015

@LeaVerou
1.) Well, that test doesn't pass.. is that a bug in Bliss.js then?
2.) Will clean up my tests. I literally copied that style from existing tests ^^"

@LeaVerou
Copy link
Owner

@AVGP

  1. Yes, it looks like you have! Congrats!
  2. Thanks!

@AVGP
Copy link
Author

AVGP commented Dec 10, 2015

@LeaVerou Hurray! The test has already payed off :P

@zdfs zdfs mentioned this pull request Dec 10, 2015
@AVGP
Copy link
Author

AVGP commented Dec 10, 2015

@LeaVerou Does this look good to you now?

@zdfs
Copy link
Collaborator

zdfs commented Dec 10, 2015

I'll merge this in since everything seems to be passing. If we need to refactor later, we will.

zdfs pushed a commit that referenced this pull request Dec 10, 2015
Adding tests for $.clone()
@zdfs zdfs merged commit bdab859 into LeaVerou:gh-pages Dec 10, 2015
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

Successfully merging this pull request may close these issues.

None yet

3 participants